Implement Distance class with arithmetic and comparison operators#2870
Implement Distance class with arithmetic and comparison operators#2870KuRaMaA4 wants to merge 3 commits into
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, although your Distance class behavior is fully correct, the type annotations currently violate the explicit checklist rule about not using string literal and tuple-like type hints. The checklist’s “Bad examples” match what you have now, for instance parameters and return types annotated like "int | float" and "Distance | int | float" in app/main.py (e.g., on the __init__, arithmetic methods, and comparison methods). Since this checklist requirement is explicit, it counts as a critical blocker that must be fixed before the solution can be accepted.
To fix this, replace all string literal type hints with real union types, for example:
- Change
km: "int | float"tokm: int | float. - Change
other: "Distance | int | float"toother: Distance | int | floatand return annotations like-> "Distance"to-> Distance.
If you are worried about forward references, you can addfrom __future__ import annotationsat the very top ofapp/main.py, which allows you to useDistancedirectly in type hints without quotes. Functionally, your solution is strong and meets the task’s behavioral requirements; once you correct the annotation style to match the “Good examples” from the checklist, it should be in great shape.
✨ 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 | float") -> None: |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:" where parameters are annotated using string literals and tuple-like syntax. Here km: "int | float" uses a string type hint instead of a proper union such as int | float. Consider changing it to km: int | float (or using from __future__ import annotations and then km: int | float).
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def _other_km(self, other: "Distance | int | float") -> "int | float": |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:" because other is annotated as a string literal type ("Distance | int | float") rather than a union type. Use other: Distance | int | float or Union[Distance, int, float] as shown in the good examples, possibly with from __future__ import annotations if needed.
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def _other_km(self, other: "Distance | int | float") -> "int | float": |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:" since the return type is annotated as the string "int | float". It should be a proper type, e.g. int | float, not a string literal.
| def _other_km(self, other: "Distance | int | float") -> "int | float": | ||
| return other.km if isinstance(other, Distance) else other | ||
|
|
||
| def __add__(self, other: "Distance | int | float") -> "Distance": |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:" for both the parameter and return annotations: other: "Distance | int | float" and -> "Distance" use string literals. Change them to proper union/Distance types (e.g. other: Distance | int | float and -> Distance).
| def __add__(self, other: "Distance | int | float") -> "Distance": | ||
| return Distance(self.km + self._other_km(other)) | ||
|
|
||
| def __iadd__(self, other: "Distance | int | float") -> "Distance": |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:" because other and the return type use string literal annotations ("Distance | int | float", "Distance"). They should be standard type hints like other: Distance | int | float and -> Distance.
| def __truediv__(self, other: "int | float") -> "Distance": | ||
| return Distance(round(self.km / other, 2)) | ||
|
|
||
| def __lt__(self, other: "Distance | int | float") -> bool: |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:" because the parameter is annotated as "Distance | int | float" (a string) instead of a proper union type. Update to other: Distance | int | float or similar.
| def __lt__(self, other: "Distance | int | float") -> bool: | ||
| return self.km < self._other_km(other) | ||
|
|
||
| def __gt__(self, other: "Distance | int | float") -> bool: |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:": other: "Distance | int | float" uses a string literal for the type. It should be a proper union type as in the checklist's good examples.
| def __gt__(self, other: "Distance | int | float") -> bool: | ||
| return self.km > self._other_km(other) | ||
|
|
||
| def __eq__(self, other: "Distance | int | float") -> bool: |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:": other: "Distance | int | float" is a string type hint. Replace it with a union type (Distance | int | float or Union[...]).
| def __eq__(self, other: "Distance | int | float") -> bool: | ||
| return self.km == self._other_km(other) | ||
|
|
||
| def __le__(self, other: "Distance | int | float") -> bool: |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:": other: "Distance | int | float" should be annotated using a proper union type, not a string literal.
| def __le__(self, other: "Distance | int | float") -> bool: | ||
| return self.km <= self._other_km(other) | ||
|
|
||
| def __ge__(self, other: "Distance | int | float") -> bool: |
There was a problem hiding this comment.
This violates checklist item #4: "Bad examples:" because other is typed as the string "Distance | int | float". Use a correct union type instead, e.g. Distance | int | float.
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 because the Distance class now fully matches the functional requirements and checklist constraints. All magic methods (__init__, __str__, __repr__, arithmetic, and comparison operators) behave correctly for both Distance instances and numeric values, including proper rounding to 2 decimals in division and correct mutation for __iadd__. The type annotations are consistent and modern, using unions (int | float) and from __future__ import annotations to support forward references, and the supported operand types for __mul__ and __truediv__ are correctly restricted to numbers as required. Comparisons are implemented cleanly via a shared helper that normalizes Distance and numeric inputs, which keeps the logic concise and avoids duplicated checks. Overall, you’ve met the task description and checklist requirements without introducing unnecessary complexity, so this implementation is ready to pass. Great work getting all these details aligned with the specifications.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.