wrote magic methods#2856
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 behaviors of your Distance class still conflict with the task’s functional requirements and checklist. On app/main.py, your __str__ currently rounds the value; it must instead output exactly "Distance: {km} kilometers." using the raw self.km (see examples like Distance(20.123)), while rounding is only required for division results. In __mul__, around the lines where you set self.km and return self, you need to stop mutating the current instance and instead return a new Distance with km equal to the product (so distance2 = distance1 * 5 leaves distance1 unchanged and makes distance2 a separate instance). Your comparison methods (__lt__, __gt__, __eq__, __le__, __ge__) currently assume other is numeric only; they must also support Distance operands, e.g. distance < Distance(60) or distance == Distance(100), by checking whether other is a Distance and then comparing self.km to other.km. In __iadd__, there is an unnecessary else branch after handling the Distance case; per the checklist [CHECKLIST ITEM #X], you should return or continue directly after the if and remove the else, letting the numeric-handling code run in the fall-through path. Once you adjust the string formatting, multiplication semantics, comparison handling, and the else style issue, your solution will align well with the specification—your use of type annotations and most arithmetic logic is solid, so you’re very close to a fully correct implementation.
✨ 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
| self.km = km | ||
|
|
||
| def __str__(self) -> str: | ||
| return f"Distance: {round(self.km, 2)} kilometers." |
There was a problem hiding this comment.
__str__ is required to output exactly "Distance: {km} kilometers." using the stored km value. Rounding in __str__ is not mentioned in the description and will change outputs like Distance(20.123) vs the expected examples.
| def __iadd__(self, other: int | Distance) -> Distance: | ||
| if isinstance(other, Distance): | ||
| self.km += other.km | ||
| else: |
There was a problem hiding this comment.
This __iadd__ implementation uses an else that isn’t needed. Checklist item "Avoid unnecessary else statements" shows the pattern of returning / continuing directly after the if instead of using else. You can remove the else and adapt the flow accordingly.
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: int) -> Distance: |
There was a problem hiding this comment.
__mul__ must return a new Distance instance with km set to the product, not mutate self. The description example uses distance2 = distance1 * 5 and then checks isinstance(distance2, Distance) and distance2.km == 100, which implies distance1 should stay unchanged.
| return self | ||
|
|
||
| def __mul__(self, other: int) -> Distance: | ||
| self.km *= other |
There was a problem hiding this comment.
Inside __mul__, you currently modify self.km and return self. This violates the requirement that distance2 is a separate Distance instance resulting from the multiplication.
| def __truediv__(self, other: float | int) -> Distance: | ||
| return Distance(round((self.km / other), 2)) | ||
|
|
||
| def __lt__(self, other: int) -> bool: |
There was a problem hiding this comment.
Comparison methods must support comparing to another Distance instance as well as to numbers. The description includes examples like distance < Distance(60) and distance > Distance(120). With the current annotation other: int and direct self.km < other, Distance operands will not be handled correctly. You need to accept both Distance and numeric types and compare using other.km when other is a Distance.
| def __lt__(self, other: int) -> bool: | ||
| return self.km < other | ||
|
|
||
| def __gt__(self, other: int) -> bool: |
There was a problem hiding this comment.
Similarly to __lt__, __gt__ must also support Distance operands (e.g. distance > Distance(120) from the description), not only int. Adjust the type annotation and logic to handle both Distance and numeric values.
| def __gt__(self, other: int) -> bool: | ||
| return self.km > other | ||
|
|
||
| def __eq__(self, other: int) -> bool: |
There was a problem hiding this comment.
__eq__ must support comparing to another Distance instance (distance == Distance(100)) as well as numbers. Currently it only accepts int and compares directly, which will fail when other is a Distance.
| def __eq__(self, other: int) -> bool: | ||
| return self.km == other | ||
|
|
||
| def __le__(self, other: int) -> bool: |
There was a problem hiding this comment.
__le__ must allow Distance vs Distance and Distance vs number comparisons (per the description examples), but it only supports int here. Consider mirroring the handling you add to __lt__.
| def __le__(self, other: int) -> bool: | ||
| return self.km <= other | ||
|
|
||
| def __ge__(self, other: int) -> bool: |
There was a problem hiding this comment.
__ge__ also needs to handle comparisons with another Distance instance in addition to numbers, like distance >= Distance(50) from the task description.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your Distance class implementation meets the functional and stylistic requirements for this task, so your solution is approved. The constructor correctly accepts a single km argument and stores it on self.km, and both __str__ and __repr__ produce the exact required string formats. Arithmetic methods (__add__, __iadd__, __mul__, __truediv__) behave as expected: they work with both other Distance instances and numeric values where required, return new Distance objects (except for the in-place add), and handle division with rounding to two decimal places. Your comparison methods properly support comparing Distance to Distance and Distance to numeric types by branching on isinstance(other, Distance) and falling back to numeric comparison. Type annotations and method signatures follow the constraints given, including excluding Distance as other in __mul__ and __truediv__, and avoiding unnecessary else blocks after returns. Overall, this is a clean, correct implementation that should pass the described tests and satisfy the checklist. Nice work on aligning the code closely with the requirements and handling the edge cases around arithmetic and comparisons.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.