Solution#2859
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 there are a few checklist violations that are considered critical for this task, even though your core functionality is correct. The return type annotations for __str__ and __repr__ in app/main.py are currently -> None, but these methods return strings; per the requirements on proper type annotations [CHECKLIST ITEM #2], both should be annotated as -> str (e.g., around the lines where these methods are defined). The signatures of __mul__ and __truediv__ only accept int, while the spec and checklist say they must support both int and float for arithmetic; update the parameter type to int | float in each of these methods [CHECKLIST ITEM #3]. Several methods (__add__, __iadd__, comparisons like __lt__, __le__, __ge__, __eq__) use an else directly after an if branch that ends with return; this matches the “bad example” for unnecessary else statements [CHECKLIST ITEM #1], so you should refactor by returning in the if block and then using a fall-through return for the other case without else. These issues don’t break your logic, but they do break explicit checklist rules and type correctness, so they need to be fixed before the solution can be accepted. Once you’ve adjusted the annotations and simplified the control flow, your implementation will fully align with the task requirements—your handling of arithmetic and comparisons is otherwise solid, so you’re very close.
✨ 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 __init__(self, km: int) -> None: | ||
| self.km = km | ||
|
|
||
| def __str__(self) -> None: |
There was a problem hiding this comment.
__str__ is annotated to return None, but it returns a string. This violates checklist item #2: "Use proper type annotations for function arguments and return types"; the return type here should be str.
| def __str__(self) -> None: | ||
| return f"Distance: {self.km} kilometers." | ||
|
|
||
| def __repr__(self) -> None: |
There was a problem hiding this comment.
__repr__ is annotated to return None, but it returns a string. This also violates checklist item #2 on proper type annotations; the return type should be str.
| def __add__(self, other: Distance | int) -> Distance: | ||
| if isinstance(other, Distance): | ||
| new_distance = self.km + other.km | ||
| else: |
There was a problem hiding this comment.
This else is directly paired with an if where the if branch ends with a return in the method. Checklist item #1 says to "Avoid unnecessary else statements" and gives a bad example matching this pattern; you can simplify by returning in the if and then handling the other case without else.
| def __iadd__(self, other: Distance | int) -> Distance: | ||
| if isinstance(other, Distance): | ||
| self.km = self.km + other.km | ||
| else: |
There was a problem hiding this comment.
Similar to __add__, this else branch is unnecessary after the if isinstance(...) because both branches simply assign then return self. This matches the "Bad example" pattern in checklist item #1 about avoiding unnecessary else statements.
| self.km = self.km + other | ||
| return self | ||
|
|
||
| def __mul__(self, other: int) -> Distance: |
There was a problem hiding this comment.
__mul__ accepts only int, but the checklist allows int | float for arithmetic operations. To align with "Support correct types", update the parameter type to int | float (and no Distance).
| def __lt__(self, other: Distance | int) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km < other.km | ||
| else: |
There was a problem hiding this comment.
This else after an if isinstance(other, Distance) is unnecessary since both branches immediately return. It matches the "Bad example" under checklist item #1; you can return in the if and then have a plain return self.km < other after it.
| def __gt__(self, other: Distance | int) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km > other.km | ||
| else: |
There was a problem hiding this comment.
Same unnecessary else pattern as in __lt__; checklist item #1 advises avoiding this structure. Consider returning in the if and handling the non-Distance case without else.
| def __eq__(self, other: Distance | int) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km == other.km | ||
| else: |
There was a problem hiding this comment.
Again, using else after an early return in the if branch matches the "Bad example" in checklist item #1. You can simplify the control flow in __eq__.
| def __le__(self, other: Distance | int) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km <= other.km | ||
| else: |
There was a problem hiding this comment.
__le__ uses the same redundant else pattern flagged by checklist item #1. Refactor to avoid else after a return path.
| def __ge__(self, other: Distance | int) -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km >= other.km | ||
| else: |
There was a problem hiding this comment.
__ge__ also follows the unnecessary else style after a return, which checklist item #1 marks as a bad example. Simplify similarly to the other comparison methods.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your Distance implementation in app/main.py meets all the specified functional and checklist requirements, so your solution is approved. The class is correctly named, __init__ stores only km in self.km, and both __str__ and __repr__ return exactly the required string formats. Arithmetic methods (__add__, __iadd__, __mul__, __truediv__) behave correctly, use appropriate type annotations, and handle both Distance and numeric operands where required, with division results rounded to two decimals. All comparison methods correctly compare by kilometers and support both Distance and numeric operands, and you’ve followed the guidance to avoid unnecessary else after return. You also respected the constraint that __mul__ and __truediv__ only accept int | float, and included from __future__ import annotations to avoid annotation-related issues. There are no critical or blocking issues remaining; functionality and requirements appear fully satisfied. Nice work aligning your code closely with the spec and keeping the implementation clean and consistent.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.