Solution#2846
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 several core requirements of the Distance class are not yet met and will cause incorrect behavior in the tests. The arithmetic methods __add__, __iadd__, __mul__, and __truediv__ are annotated to return None and use overly narrow parameter types (e.g., other: float), but the task requires them to return a Distance instance and to accept Distance | int | float where appropriate [CHECKLIST ITEM #2]. In particular, __add__ and __iadd__ must support both Distance + Distance and Distance + number, and always produce a Distance result; returning None or silently falling through when other is an unsupported type breaks the required behavior. The comparison methods (__lt__, __gt__, __eq__, __le__, __ge__) currently only compare against numeric values, so expressions like Distance(50) < Distance(60) will raise a TypeError instead of returning the expected boolean, which directly violates the specification. Additionally, __mul__ and __truediv__ have unnecessary else: return branches after a return in the if block, which matches the “bad example” pattern and is discouraged [CHECKLIST ITEM #1].
To address this, update the method signatures to use unions such as Distance | int | float for __add__/__iadd__ and int | float for __mul__/__truediv__, and ensure each returns a Distance object instead of None. Implement type-aware comparisons by checking isinstance(other, Distance) and comparing self.km to other.km in that case, and to other directly when it is numeric. Remove the extra else: return branches in __mul__ and __truediv__ so that unsupported types naturally raise errors, relying on your type annotations and task constraints. Your string methods and basic arithmetic logic are on the right track; once these type and comparison issues are fixed, your solution will align well 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 __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: float) -> None: |
There was a problem hiding this comment.
__add__ is annotated to return None and other: float, but the task requires returning a Distance for both Distance + Distance and Distance + number, and checklist item #2 expects proper annotations like Distance | int | float for other and Distance as return type.
| def __add__(self, other: float) -> None: | ||
| if isinstance(other, Distance): | ||
| return Distance(self.km + other.km) | ||
| elif isinstance(other, (int, float)): |
There was a problem hiding this comment.
This elif branch means __add__ may return None if other is neither Distance nor number, which is unspecified in the requirements; with correct type annotations you don’t need to silently return None.
| elif isinstance(other, (int, float)): | ||
| return Distance(self.km + other) | ||
|
|
||
| def __iadd__(self, other: float) -> None: |
There was a problem hiding this comment.
__iadd__ is annotated to return None and other: float, but in-place addition must support both Distance and numeric and should return Distance (self) according to the requirements and checklist item #2.
| def __iadd__(self, other: float) -> None: | ||
| if isinstance(other, Distance): | ||
| self.km += other.km | ||
| else: |
There was a problem hiding this comment.
This else branch in __iadd__ assumes any non-Distance is numeric; you should align the type annotations to reflect Distance | int | float rather than a plain float.
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: float) -> None: |
| def __truediv__(self, other: float) -> None: | ||
| if isinstance(other, (int, float)): | ||
| return Distance(round(self.km / other, 2)) | ||
| else: |
There was a problem hiding this comment.
As in __mul__, this else: return is an unnecessary else branch per checklist item #1; with proper type annotations you can remove this and rely on type-correct usage.
| else: | ||
| return | ||
|
|
||
| def __lt__(self, other: float) -> bool: |
There was a problem hiding this comment.
__lt__ only compares self.km to a numeric other; requirements demand support for comparing with another Distance as well (Distance(50) < Distance(60)), so you should handle both Distance and numeric types and update the annotation accordingly.
| def __lt__(self, other: float) -> bool: | ||
| return self.km < other | ||
|
|
||
| def __gt__(self, other: float) -> bool: |
There was a problem hiding this comment.
__gt__ has the same limitation: it doesn’t handle Distance arguments, so Distance(50) > Distance(120) will raise a TypeError instead of returning False as required.
| def __gt__(self, other: float) -> bool: | ||
| return self.km > other | ||
|
|
||
| def __eq__(self, value: float) -> bool: |
There was a problem hiding this comment.
__eq__ only supports numeric value, but the task requires equality comparison with another Distance too; adjust the annotation and logic to support both.
| def __eq__(self, value: float) -> bool: | ||
| return self.km == value | ||
|
|
||
| def __le__(self, other: float) -> bool: |
There was a problem hiding this comment.
__le__ and other comparisons should also support Distance arguments and numerics; current implementation and annotations only allow numeric values and will not satisfy the comparison requirements.
| def __ge__(self, other: "float | int | Distance") -> bool: | ||
| if isinstance(other, Distance): | ||
| return self.km >= other.km | ||
| return self.km >= other # change |
4 participants
No description provided.