Add Distance class#2858
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 there is a critical functional issue and several checklist violations that block full completion of the task. The __iadd__ method currently returns a new Distance instance instead of modifying self.km in place and returning self, which causes multiple test failures and breaks the expected += behavior (see tests around tests/test_main.py:134 and :159). You should update __iadd__ so it adjusts self.km directly (for both Distance and numeric arguments) and then returns self. In addition, checklist item [CHECKLIST ITEM #2] requires proper type annotations: km should be annotated as int | float to reflect values after division, and methods like __add__, __iadd__, comparisons, __mul__, and __truediv__ should avoid string annotations like "Distance" and instead use unions such as other: Distance | int | float and -> Distance. __mul__ and __truediv__ should also accept int | float, not just int, to match the specified behavior and the checklist’s correct examples. Once you fix the in-place addition and adjust the type hints to cover all supported argument types with proper annotations, your solution will align well with both the functional requirements and the checklist. Overall, your implementation is very close, and these changes will solidify both correctness and type safety.
✨ 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
| class Distance: | ||
| # Write your code here | ||
| pass | ||
| def __init__(self, km: int) -> None: |
There was a problem hiding this comment.
This violates checklist item #2: "Use proper type annotations for function arguments and return types". km is annotated as int, but after operations like division, you will store floats in self.km. Consider annotating as int | float (or using Union[int, float]) to reflect valid values.
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: "Distance") -> "Distance": |
There was a problem hiding this comment.
This violates checklist item #2: Bad examples use string annotations like "Distance". Here the parameter and return type are annotated as "Distance", which matches the bad pattern. Use proper type hints, e.g. other: Distance | int | float and -> Distance, possibly with from __future__ import annotations if needed.
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: "Distance") -> "Distance": |
There was a problem hiding this comment.
Checklist item #2 requires proper type annotations for all supported types. The implementation allows adding a numeric other, but the signature only declares other: "Distance". Update the annotation to include numeric types (int | float or Union[Distance, int, float]) so it matches the real behavior.
| return Distance(self.km + other.km) | ||
| return Distance(self.km + other) | ||
|
|
||
| def __iadd__(self, other: "Distance") -> "Distance": |
There was a problem hiding this comment.
This violates checklist item #2 in the same way as __add__: other is annotated only as "Distance", but you support numbers as well. Also, for __iadd__, it is more idiomatic to modify self.km in-place and return self rather than constructing a new Distance instance.
| return Distance(self.km + other.km) | ||
| return Distance(self.km + other) | ||
|
|
||
| def __mul__(self, other: int) -> "Distance": |
There was a problem hiding this comment.
__mul__ is restricted to other: int, but the checklist’s correct example for __truediv__ shows other: int | float, and multiplication logically supports floats too. Update the annotation to int | float and use a direct Distance annotation instead of "Distance" to comply with checklist item #2.
| def __mul__(self, other: int) -> "Distance": | ||
| return Distance(self.km * other) | ||
|
|
||
| def __truediv__(self, other: int) -> "Distance": |
There was a problem hiding this comment.
__truediv__ currently only accepts other: int, but the checklist item Support correct types gives a correct example def __truediv__(self, other: int | float) -> Distance:. Update the annotation to int | float and -> Distance (without quotes) to follow that requirement.
| def __truediv__(self, other: int) -> "Distance": | ||
| return Distance(round(self.km / other, 2)) | ||
|
|
||
| def __lt__(self, other: "Distance") -> bool: |
There was a problem hiding this comment.
This method (and similarly __gt__, __eq__, __le__, __ge__) uses other: "Distance" in the signature but supports both Distance and numeric types in the body. Per checklist item #2, you should annotate all supported types, e.g. other: Distance | int | float with a proper Distance type rather than a string literal.
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 there are still a few checklist violations that are considered critical for this task, even though the core behavior of Distance looks correct. The km attribute is annotated as int, but because division can produce non-integer results, it must be typed as int | float to accurately reflect possible values and comply with the type-annotations checklist. Several methods (e.g. __add__, __iadd__, comparison methods, __mul__, __truediv__) still use string annotations like "Distance" instead of proper union types such as other: Distance | int | float and return types like -> Distance, which the checklist explicitly flags as a bad pattern. In __mul__ and __truediv__, the parameter is currently annotated as int only, but the requirements specify they should accept int | float, so their signatures need to be updated to support both numeric types. In __iadd__, there is an unnecessary else after an early return in the if isinstance(other, Distance) branch; you can simplify this by returning in the if and then handling the numeric addition in the following lines without an else.
To fix everything: 1) change self.km’s annotation to int | float, 2) replace all quoted type annotations "Distance" with real types and unions (Distance | int | float where appropriate), 3) update __mul__/__truediv__ to accept int | float, and 4) remove the redundant else in __iadd__ and let the numeric case fall through. Functionally you are very close, and once these type and style fixes are in place, your solution will fully align with both the behavioral requirements and the checklist 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
| class Distance: | ||
| # Write your code here | ||
| pass | ||
| def __init__(self, km: int) -> None: |
There was a problem hiding this comment.
This violates checklist item under Type Annotations: km should be able to hold non-integer results after division, so it should be annotated as int | float instead of just int.
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: "Distance") -> "Distance": |
There was a problem hiding this comment.
This violates checklist item Type Annotations #1: using the string annotation "Distance" is a "Bad example" pattern; you should use a proper union type, e.g. other: Distance | int | float and -> Distance.
| def __iadd__(self, other: "Distance") -> "Distance": | ||
| if isinstance(other, Distance): | ||
| self.km += other.km | ||
| else: |
There was a problem hiding this comment.
This violates checklist item Make Code Easier #1: Avoid unnecessary else statements: since you return inside the if above, you can drop the else and just keep self.km += other after the if block.
| return Distance(self.km + other.km) | ||
| return Distance(self.km + other) | ||
|
|
||
| def __iadd__(self, other: "Distance") -> "Distance": |
There was a problem hiding this comment.
This violates checklist item Type Annotations #1: __iadd__ should avoid string annotations and use union types like other: Distance | int | float and -> Distance.
| self.km += other | ||
| return self | ||
|
|
||
| def __mul__(self, other: int) -> "Distance": |
There was a problem hiding this comment.
This violates checklist item Support correct types #1 and the previous review note: __mul__ should accept int | float instead of only int, and the return type should be annotated as Distance (without string quotes).
| def __mul__(self, other: int) -> "Distance": | ||
| return Distance(self.km * other) | ||
|
|
||
| def __truediv__(self, other: int) -> "Distance": |
There was a problem hiding this comment.
This violates checklist item Support correct types #1 and the previous review note: __truediv__ should accept int | float instead of only int, and the return type should be annotated as Distance (without string quotes).
| def __truediv__(self, other: int) -> "Distance": | ||
| return Distance(round(self.km / other, 2)) | ||
|
|
||
| def __lt__(self, other: "Distance") -> bool: |
There was a problem hiding this comment.
This violates checklist item Type Annotations #1: comparison methods should not use string annotations like "Distance"; use union types such as other: Distance | int | float so they reflect that they also work with numbers.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved as an educational override to help you keep progressing while you refine your style. Functionally, your Distance class is in great shape: construction, string/representation methods, arithmetic (__add__, __iadd__, __mul__, __truediv__), and comparisons with both Distance and numeric values all behave as required. One minor style issue remains in app/main.py within __iadd__: after the if isinstance(other, Distance): branch (around that line), you still use an else even though the if branch already returns, which goes against the guideline to avoid unnecessary else after a return [CHECKLIST ITEM #1]. This matters because removing the redundant else makes the control flow clearer and more idiomatic:
if isinstance(other, Distance):
self.km += other.km
return self
self.km += other
return selfOverall, you’ve done well implementing the required behaviors and type annotations; now you just need that small cleanup to fully match the style checklist. Keep up the good work—this kind of polish is a normal and valuable part of learning clean, professional Python.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
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 "Avoid unnecessary else statements": you have an else block after the if isinstance(other, Distance) that already performs the Distance-specific logic and then fall through to return self. You can simplify by handling the Distance case inside the if and then performing numeric addition in the following lines without else.
2 participants
No description provided.