Tasks completed#2862
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 are a few checklist violations that count as critical blockers for this task, even though your functional behavior is largely correct. Specifically, several methods in app/main.py use an else after an early return (e.g., __add__, __eq__, __lt__, __gt__, __le__, __ge__), which goes against [CHECKLIST ITEM #1]; you should remove the else and let the second return be the default fallthrough path. In multiple method signatures (e.g., __add__, __mul__, __truediv__, __iadd__), the return type is annotated as the string "Distance" instead of Distance, which violates [CHECKLIST ITEM #2] given that from __future__ import annotations is already in use. Also, __mul__ and __truediv__ currently accept Distance in their parameter types, but per [CHECKLIST ITEM #3] they must only accept numeric values, so change the parameter annotation to int | float and ensure the implementation reflects that constraint.
The core arithmetic and comparison logic looks solid and matches the described behavior for both Distance and numeric inputs, which is great. Once you update the unnecessary else branches, fix the type annotations to use Distance directly, and restrict __mul__/__truediv__ to numeric arguments, your solution will satisfy both the functional requirements and the checklist. Keep going—you’re very close; these are mainly structural and type-annotation refinements that will strengthen your code quality and adherence to the project 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
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: int | float | Distance) -> "Distance": |
There was a problem hiding this comment.
This violates checklist item #2: "Use proper type annotations for function arguments and return types" because the return type is annotated as a string "Distance" instead of directly using Distance, especially given from __future__ import annotations at the top.
Consider changing the signature to def __add__(self, other: int | float | Distance) -> Distance:.
| if isinstance(other, Distance): | ||
| return Distance(self.km + other.km) | ||
| else: |
There was a problem hiding this comment.
This violates checklist item #1: "Avoid unnecessary else statements". You return inside the if branch, so the else is not needed.
Refactor to:
if isinstance(other, Distance):
return Distance(self.km + other.km)
return Distance(self.km + other)| else: | ||
| return Distance(self.km + other) | ||
|
|
||
| def __mul__(self, other: int | float | Distance) -> "Distance": |
There was a problem hiding this comment.
This violates checklist item #3: "__truediv__ and __mul__ should not accept Distance instance as argument". The parameter type currently includes Distance.
Update the annotation to other: int | float so this method only accepts numeric values.
| else: | ||
| return Distance(self.km + other) | ||
|
|
||
| def __mul__(self, other: int | float | Distance) -> "Distance": |
There was a problem hiding this comment.
| def __mul__(self, other: int | float | Distance) -> "Distance": | ||
| return Distance(self.km * other) | ||
|
|
||
| def __truediv__(self, other: int | float | Distance) -> "Distance": |
There was a problem hiding this comment.
This violates checklist item #3: "__truediv__ and __mul__ should not accept Distance instance as argument". The parameter type currently includes Distance.
Change the signature to def __truediv__(self, other: int | float) -> Distance:.
| if isinstance(other, Distance): | ||
| return self.km > other.km | ||
| else: |
There was a problem hiding this comment.
This violates checklist item #1: unnecessary else after a return. You can remove else and let the second return be the default path.
Refactor to:
if isinstance(other, Distance):
return self.km > other.km
return self.km > other| else: | ||
| return self.km > other | ||
|
|
||
| def __le__(self, other: int | float | Distance) -> bool: |
There was a problem hiding this comment.
This violates checklist item #2: the return type is annotated as "Distance" instead of Distance. Replace the string with the class name.
Also, note the extra space before : in else : is not needed, though this is a minor style issue compared to the checklist requirements.
| if isinstance(other, Distance): | ||
| return self.km <= other.km | ||
| else : |
There was a problem hiding this comment.
This violates checklist item #1: you have an unnecessary else after a return in the if branch.
You can write:
if isinstance(other, Distance):
return self.km <= other.km
return self.km <= other| else : | ||
| return self.km <= other | ||
|
|
||
| def __ge__(self, other: int | float | Distance) -> bool: |
There was a problem hiding this comment.
This violates checklist item #2: the return type is annotated as "Distance" instead of Distance. Update to -> Distance for proper type annotations.
| if isinstance(other, Distance): | ||
| return self.km >= other.km | ||
| else: |
There was a problem hiding this comment.
This violates checklist item #1: the else is unnecessary because you already return in the if branch.
Simplify to:
if isinstance(other, Distance):
return self.km >= other.km
return self.km >= other
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it fully implements the Distance class as required and behaves correctly for all specified operations. The constructor, __str__, and __repr__ produce the expected formats, and arithmetic methods handle both Distance instances and numeric values correctly, including in-place addition. Your __mul__ and __truediv__ methods are restricted to numeric (int | float) arguments, and division results are properly rounded to two decimal places. All comparison operators work as intended for both Distance-to-Distance and Distance-to-number comparisons, returning accurate boolean results without unnecessary branches after early returns. The use of from __future__ import annotations and direct Distance type annotations matches the checklist and avoids forward-reference issues. Overall, the code is clear, functional, and meets the task’s functional and style requirements—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
3 participants
No description provided.