Code write #2308
Code write #2308
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good job on implementing the core functionality of the Distance class, especially with __init__, __str__, and __add__. However, some changes are needed before this can be approved.
The most critical issue is that all comparison methods (__lt__, __gt__, etc.) will raise a TypeError when comparing two Distance objects. You need to handle the case where the other argument is a Distance instance and compare its km attribute. Additionally, the __mul__ and __truediv__ methods incorrectly accept Distance objects; their type hints should be int | float, and the isinstance checks should be removed as per checklist item #3.1. Lastly, please remove the unnecessary else blocks where the preceding if block contains a return statement [CHECKLIST ITEM #1.1].
Addressing these points will make your solution fully compliant with the requirements.
✨ 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 __iadd__(self, other: Distance | int | float) -> Distance: | ||
| if isinstance(other, Distance): | ||
| self.km += other.km | ||
| else: |
There was a problem hiding this comment.
This violates checklist item #1.1: 'Avoid unnecessary else statements'. Since the if block on line 25 does not contain a return statement, this else block is acceptable. My apologies for the confusion.
app/main.py
Outdated
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: Distance | int | float) -> Distance | None: |
There was a problem hiding this comment.
This violates checklist item #3.1: '__truediv__ and __mul__ should not accept Distance instance as argument.' The type hint for other should be int | float, and the return type should be just Distance. With the correct type hint, the isinstance check on the next line is no longer needed.
app/main.py
Outdated
| def __mul__(self, other: Distance | int | float) -> Distance | None: | ||
| if isinstance(other, Distance): | ||
| return None | ||
| else: |
There was a problem hiding this comment.
This else block is unnecessary. This violates checklist item #1.1 because the preceding if block contains a return statement. You can remove the else and unindent its content.
app/main.py
Outdated
| self.km * other | ||
| ) | ||
|
|
||
| def __truediv__(self, other: Distance | int | float) -> Distance | None: |
There was a problem hiding this comment.
This violates checklist item #3.1: '__truediv__ and __mul__ should not accept Distance instance as argument.' The type hint for other should be int | float, and the return type should be Distance. Consequently, the isinstance check on the next line is not needed.
app/main.py
Outdated
| def __truediv__(self, other: Distance | int | float) -> Distance | None: | ||
| if isinstance(other, Distance): | ||
| return None | ||
| else: |
There was a problem hiding this comment.
This else block violates checklist item #1.1. Since the if block returns, the else is redundant and the code within it can be unindented.
| return Distance(round(self.km / other, 2)) | ||
|
|
||
| def __lt__(self, other: Distance | int | float) -> bool: | ||
| return self.km < other |
There was a problem hiding this comment.
The current implementation for comparison will raise a TypeError when comparing a Distance object with another Distance object (e.g., self.km < Distance(...)). You need to handle the case where other is a Distance instance and compare its km attribute. This same issue applies to all comparison methods (__gt__, __eq__, __le__, __ge__).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the feedback from the previous review, especially in making the comparison methods work with Distance objects. Your solution is very close to being complete, but there are a couple of critical issues with type hints and redundant code that need to be fixed.
Here are the required changes:
-
In
__mul__and__truediv__, theif isinstance(other, Distance):check is unnecessary and incorrect. These methods should only operate on numbers as per the requirements [CHECKLIST ITEM #3.1]. Please remove theseifblocks and update the return type hint fromDistance | Noneto justDistance. -
The type hints for the comparison methods (
__lt__,__gt__, etc.) areother: int | float, but your implementation correctly handlesDistanceobjects too. To make the type hints match the implementation, please update them toother: Distance | int | float.
Fixing these inconsistencies will make your solution fully correct. You're almost there!
✨ 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 __mul__(self, other: int | float) -> Distance | None: | ||
| if isinstance(other, Distance): | ||
| return None | ||
| return Distance(self.km * other) |
There was a problem hiding this comment.
The return type here should be just Distance, not Distance | None. The check if isinstance(other, Distance): is unnecessary and should be removed. Checklist item #3.1 specifies that this method should only accept numeric types, which your type hint for other already correctly enforces. The isinstance check contradicts this and adds logic that shouldn't be reachable.
app/main.py
Outdated
| def __truediv__(self, other: int | float) -> Distance | None: | ||
| if isinstance(other, Distance): | ||
| return None | ||
| return Distance(round(self.km / other, 2)) |
There was a problem hiding this comment.
Similar to __mul__, the return type should be Distance, and the if isinstance(other, Distance): block should be removed. The type hint for other correctly restricts it to numeric types, so this check is redundant.
app/main.py
Outdated
| return None | ||
| return Distance(round(self.km / other, 2)) | ||
|
|
||
| def __lt__(self, other: int | float) -> bool: |
There was a problem hiding this comment.
The type hint for other is int | float, but your implementation correctly handles Distance objects as well, which is required by the task. To make the type hint match the implementation, it should be other: Distance | int | float. This comment applies to all comparison methods (__gt__, __eq__, __le__, __ge__).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this submission! You've done a fantastic job addressing all the feedback from the previous review. The type hints are now spot-on, and the __mul__ and __truediv__ methods correctly handle only numeric types. Your implementation is clean, correct, and meets all the requirements perfectly. I'm happy to approve your solution. Well done!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.