Solution #2303
Solution #2303
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the Distance class! The logic for all the arithmetic and comparison operations is correct.
However, changes are needed for the return type annotations on your arithmetic methods (__add__, __iadd__, __mul__, __truediv__). They are currently annotated as -> Distance | int | float, but these methods will only ever return a Distance instance or NotImplemented. This violates a core requirement of the task [CHECKLIST ITEM #2.1].
Please update the return type hint for these methods to -> Distance to accurately reflect what they return. Once that's fixed, this will be ready to go!
✨ 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
app/main.py
Outdated
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: Distance | int | float) -> Distance | int | float: |
There was a problem hiding this comment.
This violates checklist item #2.1: 'Use proper type annotations for function arguments and return types'.
The return type hint is not quite right. This method is expected to return a new Distance instance (or NotImplemented). It will never return an int or float directly. The correct return type should just be Distance.
app/main.py
Outdated
| return Distance(total_km) | ||
|
|
||
| def __iadd__(self, other: Distance | int | float) \ | ||
| -> Distance | int | float: |
There was a problem hiding this comment.
This violates checklist item #2.1: 'Use proper type annotations for function arguments and return types'.
Similar to __add__, the return type hint here should be more specific. Since this is an in-place operation, it should return an instance of the class itself, which is Distance. The | int | float part is incorrect.
app/main.py
Outdated
| return NotImplemented | ||
| return self | ||
|
|
||
| def __mul__(self, other: int | float) -> Distance | int | float: |
There was a problem hiding this comment.
This violates checklist item #2.1: 'Use proper type annotations for function arguments and return types'.
The return type hint here is incorrect. This method should return a new Distance object, not an int or float. Please review the type annotation guidelines in the checklist.
app/main.py
Outdated
| return NotImplemented | ||
| return Distance(total_km) | ||
|
|
||
| def __truediv__(self, other: int | float) -> Distance | int | float: |
There was a problem hiding this comment.
This violates checklist item #2.1: 'Use proper type annotations for function arguments and return types'.
Just like the other arithmetic methods, this method's return type hint should be Distance, as it returns a new Distance instance. The current annotation -> Distance | int | float is inaccurate.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing the Distance class! The code is very well-structured and handles almost all cases correctly.
However, this requires a small but important correction in two of your comparison methods to ensure they behave correctly with unsupported types.
- In your
__le__and__ge__methods, you should returnNotImplementedfor unsupported types instead ofFalse. ReturningFalseincorrectly suggests that a comparison likeDistance(10) <= "foo"is a valid operation that evaluates to false. The correct behavior is to signal that the operation is undefined, which allows Python to raise aTypeError. This will make these methods consistent with how you've correctly implemented__lt__.
Once you've updated the return value for these methods, your solution will be perfect!
✨ 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
app/main.py
Outdated
| def __add__(self, other: Distance | int | float) -> Distance: | ||
| if isinstance(other, Distance): | ||
| total_km = self.km + other.km | ||
| elif isinstance(other, (int, float)): | ||
| total_km = self.km + other | ||
| else: | ||
| return NotImplemented | ||
| return Distance(total_km) |
There was a problem hiding this comment.
While this implementation is correct, the arithmetic methods (__add__, __iadd__, __mul__, __truediv__) could be simplified to match the cleaner style you've used for the comparison methods. Consider returning the result directly from within the if/elif blocks, which removes the need for the total_km variable and the else block. This aligns with the principle in checklist item #1.1 to make the code easier to follow.
app/main.py
Outdated
| return self.km <= other.km | ||
| elif isinstance(other, (int, float)): | ||
| return self.km <= other | ||
| return False |
There was a problem hiding this comment.
This should return NotImplemented for unsupported types. Returning False implies a defined-but-false comparison (e.g., Distance(10) <= "foo" would be False), when the operation should be considered undefined. Returning NotImplemented allows Python to raise a TypeError, which is the correct behavior and consistent with your __lt__ method.
app/main.py
Outdated
| return self.km >= other.km | ||
| elif isinstance(other, (int, float)): | ||
| return self.km >= other | ||
| return False |
There was a problem hiding this comment.
Similar to __le__, this should return NotImplemented for unsupported types to ensure consistent and correct behavior for all ordering comparisons.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this task! I am approving your solution so you can continue to the next stage, but there is one small style improvement to consider.
You did an excellent job correcting the comparison methods to return NotImplemented for unsupported types. The logic is now perfect.
For future reference, the __iadd__ method in app/main.py contains an else block that isn't necessary. Since the if and elif branches both return, you can remove the else and simply have return NotImplemented as the final line of the method. This is a common pattern for making code cleaner and follows the project's style guide. [CHECKLIST ITEM #1 under 'Make Code Easier']
Overall, this is a fantastic submission. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| else: | ||
| return NotImplemented |
There was a problem hiding this comment.
This violates checklist item #1 under 'Make Code Easier': 'Avoid unnecessary else statements'. The structure of this method can be simplified to avoid this else block. Consider using a guard clause at the top of the function to handle unsupported types first.
2 participants
No description provided.