Solution #1807
Solution #1807
Conversation
There was a problem hiding this comment.
Thank you for your hard work on the Distance class implementation! 🌟 While your code meets most of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Type Annotations: Ensure that all methods consistently use the correct type annotations for the
otherparameter. Specifically, the type should beint | float | Distanceto match the logic inside the methods. This will improve the clarity and correctness of your code. -
Return Type Consistency: The return type for comparison methods should be
boolinstead ofbool | None, as they should always return a boolean value. This change will align the method signatures with their intended behavior. -
Redundant Checks in
__truediv__: Simplify the logic in the__truediv__method by removing redundant checks. Combine the conditions to ensure thatotheris a valid type and not zero in a single statement.
These changes are important to ensure the functionality and maintainability of your code. Once you've made these adjustments, your implementation will be in great shape! Keep up the great work, and don't hesitate to reach out if you have any questions or need further guidance. 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
app/main.py
Outdated
| def __truediv__(self, other: int | float) -> Distance | None: | ||
| if isinstance(other, (Distance, int, float)) and other != 0: | ||
| return Distance(round(self.km / other, 2)) | ||
| if isinstance(other, Distance) and other != 0: | ||
| return Distance(round(self.km / other.km, 2)) |
There was a problem hiding this comment.
The __truediv__ method has redundant checks. The first condition checks if other is a Distance, int, or float and not zero, but the second condition checks if other is a Distance and not zero again. This redundancy can be removed by combining the logic into a single condition.
app/main.py
Outdated
| if isinstance(other, Distance) and other != 0: | ||
| return Distance(round(self.km / other.km, 2)) | ||
|
|
||
| def __lt__(self, other: int | float) -> bool | None: |
There was a problem hiding this comment.
The return type annotation bool | None is misleading. The method should always return a boolean value, so it should be bool instead of bool | None.
app/main.py
Outdated
| other.km if isinstance(other, Distance) else other | ||
| ) | ||
|
|
||
| def __gt__(self, other: int | float) -> bool | None: |
There was a problem hiding this comment.
Similar to the __lt__ method, the return type annotation bool | None is misleading. The method should always return a boolean value, so it should be bool instead of bool | None.
app/main.py
Outdated
| other.km if isinstance(other, Distance) else other | ||
| ) | ||
|
|
||
| def __eq__(self, other: int) -> bool | None: |
There was a problem hiding this comment.
The type annotation for other should be int | float | Distance to match the logic inside the method.
app/main.py
Outdated
| other.km if isinstance(other, Distance) else other | ||
| ) | ||
|
|
||
| def __le__(self, other: int) -> bool | None: |
There was a problem hiding this comment.
The type annotation for other should be int | float | Distance to match the logic inside the method.
app/main.py
Outdated
| def __le__(self, other: int) -> bool | None: | ||
| return self < other or self == other | ||
|
|
||
| def __ge__(self, other: int) -> bool | None: |
There was a problem hiding this comment.
The type annotation for other should be int | float | Distance to match the logic inside the method.
There was a problem hiding this comment.
Great job on improving the Distance class implementation! 🎉 While there are still a few issues to address, such as ensuring consistent type annotations and simplifying logic in the __truediv__ method, your progress is commendable. Please review the comments to refine your code further. Remember, addressing these points will enhance the functionality and maintainability of your code. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| def __repr__(self) -> str: | ||
| return f"Distance(km={self.km})" | ||
|
|
||
| def __add__(self, other: int | float) -> "Distance": |
There was a problem hiding this comment.
The type annotation for the other parameter should include Distance as well, i.e., int | float | Distance, to match the logic inside the method.
| return Distance(self.km + other) | ||
| return NotImplemented | ||
|
|
||
| def __iadd__(self, other: int | float) -> "Distance": |
There was a problem hiding this comment.
The type annotation for the other parameter should include Distance as well, i.e., int | float | Distance, to match the logic inside the method.
| return self | ||
| return NotImplemented | ||
|
|
||
| def __mul__(self, other: int | float) -> "Distance" | None: |
There was a problem hiding this comment.
The return type should be Distance instead of Distance | None since the method should always return a Distance object when other is a valid type.
| if isinstance(other, (int, float)): | ||
| return Distance(self.km * other) | ||
|
|
||
| def __truediv__(self, other: int | float) -> Distance | None: |
There was a problem hiding this comment.
The return type should be Distance instead of Distance | None since the method should always return a Distance object when other is a valid type.
|
|
||
| def __truediv__(self, other: int | float) -> Distance | None: | ||
| if isinstance(other, (int, float)): | ||
| return Distance(round(self.km / other, 2)) if other != 0 else None |
There was a problem hiding this comment.
Simplify the logic by combining the type check and zero check in a single statement. You can use: if isinstance(other, (int, float)) and other != 0:.
| other.km if isinstance(other, Distance) else other | ||
| ) | ||
|
|
||
| def __eq__(self, other: int | float | Distance) -> bool | None: |
There was a problem hiding this comment.
The return type should be bool instead of bool | None since the method should always return a boolean value.
| other.km if isinstance(other, Distance) else other | ||
| ) | ||
|
|
||
| def __le__(self, other: int | float | Distance) -> bool | None: |
There was a problem hiding this comment.
The return type should be bool instead of bool | None since the method should always return a boolean value.
| def __le__(self, other: int | float | Distance) -> bool | None: | ||
| return self < other or self == other | ||
|
|
||
| def __ge__(self, other: int | float | Distance) -> bool | None: |
There was a problem hiding this comment.
The return type should be bool instead of bool | None since the method should always return a boolean value.
No description provided.