Solve#2855
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because, while your Distance class meets the functional requirements, it does not yet satisfy several explicit checklist and linting requirements that are considered critical for this task. Multiple magic methods in app/main.py still use other: Any (e.g., __add__, __iadd__, comparisons) and lack precise return type annotations, which violates the checklist’s guidance on proper type annotations and causes flake8-annotations errors on lines such as 5, 8, 11, 14, 22, and others. You also have __mul__ and __truediv__ accepting Any instead of being restricted to int | float, which conflicts with the requirement that these methods must not accept Distance instances as operands. In several places, you use if/elif followed by an else that returns None (for arithmetic and comparisons), which goes against the checklist item "Avoid unnecessary else statements" and can lead to unclear behavior for unsupported types.
To fix this, update the signatures so that methods like __add__/__iadd__ and all comparison methods use other: Distance | int | float and -> Distance or -> bool as appropriate, and change __mul__/__truediv__ to other: int | float -> Distance. Then, remove the trailing else/return None branches and rely on the annotated types instead—if you need to handle invalid types, prefer raising TypeError rather than silently returning None. Once these type annotations and control-flow structures are adjusted, your solution will be fully aligned with both the functional spec and the checklist. Overall, you’ve done a solid job on behavior; these changes are mainly about meeting the project’s strict type and style standards, and they’re a good opportunity to practice precise typing and cleaner branching logic.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| def __repr__(self): | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: Any): |
There was a problem hiding this comment.
other is annotated as Any, but the checklist requires proper type annotations. For __add__, it should be something like other: Distance | int | float instead of Any to match checklist item "Use proper type annotations for function arguments and return types".
| elif isinstance(other, int | float): | ||
| return Distance(self.km + other) | ||
| else: |
There was a problem hiding this comment.
This elif plus else structure leads to an unnecessary else branch that returns None. The checklist item "Avoid unnecessary else statements" shows that after handling valid cases, you should fall through to a final return without else rather than using else:.
| else: | ||
| return None | ||
|
|
||
| def __iadd__(self, other: Any): |
There was a problem hiding this comment.
other is annotated as Any, but for in-place addition the checklist expects specific types; here you should annotate as other: Distance | int | float instead of Any to satisfy the proper type annotations requirement.
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: Any): |
There was a problem hiding this comment.
other is annotated as Any, but checklist item "__truediv__ and __mul__ should not accept Distance instance as argument" expects signatures like def __mul__(self, other: int | float) -> Distance:. Using Any does not enforce this and violates the intent of that checklist item.
| if isinstance(other, int | float): | ||
| return Distance(self.km * other) | ||
| else: |
There was a problem hiding this comment.
This else returning None is an unnecessary else branch after an if that handles the only valid types. Checklist item "Avoid unnecessary else statements" suggests dropping the else: and using a simple fall-through return or error handling if needed.
| elif isinstance(other, int | float): | ||
| return self.km < other | ||
| return None |
There was a problem hiding this comment.
This final return None in comparison methods is unnecessary and results from using Any plus manual type filtering. The checklist’s note that explicit type checking isn’t required when annotations are correct suggests you should rely on precise annotations and avoid such fallback returns.
| return self.km < other | ||
| return None | ||
|
|
||
| def __le__(self, other: Any): |
There was a problem hiding this comment.
Same issue as __lt__: other: Any and a final return None do not comply with the checklist emphasis on proper type annotations and avoiding extra explicit type checking when annotations are correct.
| return self.km <= other | ||
| return None | ||
|
|
||
| def __gt__(self, other: Any): |
There was a problem hiding this comment.
other is typed as Any here as well; per the type-annotation checklist, this should be narrowed to Distance | int | float, and you can remove the return None fallback once the types are constrained.
| return self.km > other | ||
| return None | ||
|
|
||
| def __ge__(self, other: Any): |
There was a problem hiding this comment.
Again, other: Any plus return None for unsupported types goes against the checklist guidance. Tighten the annotation to accepted types and drop the fallback None to keep comparisons consistent and type-safe.
| return self.km >= other | ||
| return None | ||
|
|
||
| def __eq__(self, other: Any): |
There was a problem hiding this comment.
__eq__ follows the same pattern with other: Any and return None. To comply with checklist item on type annotations and the note about not needing explicit type checks, annotate other as Distance | int | float and return a bool for valid comparisons without using None.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a direct violation of the explicit control-flow style requirement about avoiding unnecessary else/elif branches when there is a final fallback. Functionally, your Distance class looks great: initialization, string/repr output, arithmetic, rounding, comparisons, and type annotations all behave as required and match the task description.
The remaining critical issue is stylistic but explicitly mandated: several methods (e.g. __add__, __lt__, __le__, __gt__, __ge__, __eq__ in app/main.py) still follow the if ... elif ... plus trailing return False/raise TypeError pattern that [CHECKLIST ITEM #1] identifies as a bad example. This must be refactored so that each supported type is handled in its own if isinstance(...) branch that returns directly, followed by a single final fallback after those if blocks (for example, raise TypeError("Unsupported operand type") or return False) without using elif. As a concrete hint, for a comparison method you can do:
if isinstance(other, Distance): return self.km < other.kmif isinstance(other, (int, float)): return self.km < float(other)return False(orraise TypeError) as the last statement.
Once you update all these methods to follow that structure, your solution will fully satisfy both the functional requirements and the checklist style rules. You’ve done solid work on the functionality and typing—this last refactor is small but important to align with the prescribed style, and you’re very close to a complete, fully compliant solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| def __add__(self, other: Distance | int | float) -> Distance: | ||
| if isinstance(other, Distance): | ||
| return Distance(self.km + other.km) | ||
| elif isinstance(other, int | float): | ||
| return Distance(self.km + other) | ||
| raise TypeError |
There was a problem hiding this comment.
This method still uses an elif and a trailing raise TypeError after the if, which goes against checklist item "Avoid unnecessary else statements" and the previous guidance to remove trailing branches returning/raising after if/elif. You can structure it as if isinstance(other, Distance): ...; if isinstance(other, (int, float)): ...; raise TypeError without elif.
| def __lt__(self, other: Distance | int | float) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km < other.km | ||
| elif isinstance(other, int | float): | ||
| return self.km < other | ||
| return False |
There was a problem hiding this comment.
The comparisons use elif followed by a final return False, which matches the "Bad example" pattern in checklist item "Avoid unnecessary else statements". Consider using separate if isinstance(...) checks and then a single fallback (e.g. raise TypeError or returning False) without elif.
| def __le__(self, other: Distance | int | float) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km <= other.km | ||
| elif isinstance(other, int | float): | ||
| return self.km <= other | ||
| return False |
There was a problem hiding this comment.
Same issue as __lt__: the elif plus trailing return False pattern violates the checklist guidance to avoid unnecessary else/elif branches. Refactor to multiple if checks with a single fallback.
| def __gt__(self, other: Distance | int | float) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km > other.km | ||
| elif isinstance(other, int | float): | ||
| return self.km > other | ||
| return False |
There was a problem hiding this comment.
Here too, the control flow uses elif followed by a default return False. To fully comply with the checklist, remove the unnecessary elif and structure the logic as separate if checks plus one fallback.
| def __ge__(self, other: Distance | int | float) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km >= other.km | ||
| elif isinstance(other, int | float): | ||
| return self.km >= other | ||
| return False |
There was a problem hiding this comment.
This method repeats the elif + return False pattern the checklist warns against; refactor to avoid the elif and have a clear final branch instead.
| def __eq__(self, other: Distance | int | float) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km == other.km | ||
| elif isinstance(other, int | float): | ||
| return self.km == other | ||
| return False |
There was a problem hiding this comment.
The equality method mirrors the same unnecessary elif + default return pattern as other comparisons, which conflicts with checklist item "Avoid unnecessary else statements". Refactoring here in the same way will bring all comparison methods in line with the style requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your Distance class implementation is solid and fully matches the described requirements: initialization stores km correctly, __str__ and __repr__ return the exact required formats, arithmetic operations behave as expected, and comparisons work with both Distance instances and numeric values. The type annotations are clean and modern (using from __future__ import annotations and int | float | Distance), and your control flow avoids unnecessary else/elif chains before the final fallback, which aligns with the style guidance [CHECKLIST ITEM #1]. The restrictions on __mul__ and __truediv__ to numeric types and the explicit rounding to two decimals in division are implemented correctly and clearly.
Your solution is approved; this is a strong, well-structured implementation that demonstrates good understanding of magic methods, typing, and clean control flow. Keep this level of precision and clarity in future tasks as well.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.