Solution#2796
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation is functionally correct with all magic methods working as expected, but there are explicit checklist violations that need addressing. First, [CHECKLIST ITEM #1] violations: remove all unnecessary else statements after isinstance() checks in methods like __add__, __iadd__, __mul__, __truediv__, __lt__, __gt__, __eq__, __le__, and __ge__ — restructure to early returns or direct returns without the else block. Second, [CHECKLIST ITEM #4] violations: the type annotations for __mul__ and __truediv__ incorrectly include Distance in the union type (Distance | float should be int | float), and the isinstance checks with return None contradict the intended behavior — simply remove Distance from the type annotation and the isinstance check entirely. The core functionality is solid, so these are straightforward fixes to align with the coding standards.
✨ 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
| if isinstance(other, Distance): | ||
| return Distance(self.km + other.km) | ||
| else: |
There was a problem hiding this comment.
Checklist item #1 violation: Avoid unnecessary else statements. The else here can be removed since the if block returns. Good pattern: if isinstance(...): return ... followed by return ...
| if isinstance(other, Distance): | ||
| self.km += other.km | ||
| else: |
There was a problem hiding this comment.
Checklist item #1 violation: Unnecessary else statement. Can be simplified by removing else.
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: Distance | float) -> Distance | None: |
There was a problem hiding this comment.
Checklist item #4 violation: Type annotation includes Distance but should only be int | float. The isinstance check with return None contradicts proper type handling.
| if isinstance(other, Distance): | ||
| return None |
There was a problem hiding this comment.
Checklist item #1 violation: Unnecessary else statement. Can be simplified.
| else: | ||
| return Distance(self.km * other) | ||
|
|
||
| def __truediv__(self, other: Distance | float) -> Distance | None: |
There was a problem hiding this comment.
Checklist item #4 violation: Type annotation includes Distance but should only be int | float. Also return None is incorrect - should either raise an error or the type annotation should exclude Distance entirely.
| if isinstance(other, Distance): | ||
| return self.km < other.km | ||
| else: |
There was a problem hiding this comment.
Checklist item #1 violation: Unnecessary else statement.
| if isinstance(other, Distance): | ||
| return self.km > other.km | ||
| else: |
There was a problem hiding this comment.
Checklist item #1 violation: Unnecessary else statement.
| if isinstance(other, Distance): | ||
| return self.km == other.km | ||
| else: |
There was a problem hiding this comment.
Checklist item #1 violation: Unnecessary else statement.
| if isinstance(other, Distance): | ||
| return self.km <= other.km | ||
| else: |
There was a problem hiding this comment.
Checklist item #1 violation: Unnecessary else statement.
| if isinstance(other, Distance): | ||
| return self.km >= other.km | ||
| else: |
There was a problem hiding this comment.
Checklist item #1 violation: Unnecessary else statement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is functionally correct with all magic methods working as expected, including arithmetic operations, comparisons, and string representations. The code properly handles Distance and numeric operands across all methods. There are two minor type annotation issues: __mul__ and __truediv__ (lines 23 and 26) include | None in their return types, but neither method ever returns None - these should simply be -> Distance. Additionally, consider adding from __future__ import annotations at the top of the file for better type annotation handling. These are cosmetic improvements rather than functional issues, so your solution meets the requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: int | float) -> Distance | None: |
There was a problem hiding this comment.
Return type annotation includes | None but the method always returns Distance. Change to -> Distance to match the actual behavior.
| def __mul__(self, other: int | float) -> Distance | None: | ||
| return Distance(self.km * other) | ||
|
|
||
| def __truediv__(self, other: int | float) -> Distance | None: |
There was a problem hiding this comment.
Return type annotation includes | None but the method always returns Distance. Change to -> Distance to match the actual behavior.
2 participants
No description provided.