-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
solution #1261
base: master
Are you sure you want to change the base?
solution #1261
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your code so far! However, I suggest making a few changes. Please, avoid using mutable types like lists as default values. Also, consider raising exceptions instead of returning strings in case of errors. Keep up the good work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
class BaseRobot: | ||
def __init__( | ||
self, name: str, weight: int, coords: list[int] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to avoid mutable default arguments like list
in function definitions, as they can lead to unexpected results. A better approach would be to set the default value to None
and then assign the default value within the function if it's not provided.
|
||
class FlyingRobot(BaseRobot): | ||
def __init__( | ||
self, name: str, weight: int, coords: list[int] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue with mutable default arguments as in the BaseRobot
class. Please, avoid using mutable types as default values.
app/main.py
Outdated
class DeliveryDrone(FlyingRobot): | ||
def __init__( | ||
self, name: str, weight: int, max_load_weight: int, | ||
coords: list[int] = None, current_load: Cargo = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue with mutable default arguments as in the BaseRobot
class. Please, avoid using mutable types as default values.
def hook_load(self, cargo: Cargo) -> str: | ||
if self.current_load is not None: | ||
return "Cannot hook load. There is already cargo hooked." | ||
elif cargo.weight > self.max_load_weight: | ||
return ( | ||
f"Cannot hook load. The cargo is too heavy: " | ||
f"{cargo.weight} > {self.max_load_weight}." | ||
) | ||
self.current_load = cargo | ||
return f"Cargo weighing {cargo.weight} hooked successfully." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hook_load
method currently returns a string to indicate the status of the operation. This is not a common practice in Python. It would be better to raise an exception in case of an error. This way, the calling code can decide what to do in case of an error (e.g., show a message to the user, try again, etc.).
def unhook_load(self) -> str: | ||
if self.current_load is None: | ||
return "No cargo to unhook." | ||
|
||
unhooked_cargo = self.current_load | ||
self.current_load = None | ||
return f"Cargo weighing {unhooked_cargo.weight} unhooked successfully." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the hook_load
method, the unhook_load
method should raise an exception instead of returning a string in case of an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've done a great job so far, but there's a recurring issue in your code that needs to be addressed. You've used mutable objects as default arguments in your function definitions, which can lead to unexpected behavior. Don't worry, it's a common pitfall in Python and easy to fix! Just replace the mutable default arguments with non-mutable ones (like None
) and assign the default value within the function if no value was provided. Keep up the good work! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
self, name: str, weight: int, coords: list[int] = None | ||
) -> None: | ||
if coords is None: | ||
coords = [0, 0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default mutable arguments in function definitions can lead to unexpected behavior. In Python, default mutable objects are shared between subsequent function calls which can lead to unexpected behaviour. It would be better to use a non-mutable default argument, such as None
, and then assign the default value within the function if no value was provided.
app/main.py
Outdated
self, name: str, weight: int, coords: list[int] = None | ||
) -> None: | ||
super().__init__( | ||
name, weight, coords if coords is not None else [0, 0, 0] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same issue is here as mentioned above. You should avoid using mutable default arguments in function definitions.
app/main.py
Outdated
self, name: str, weight: int, max_load_weight: int, | ||
coords: list[int] = None, current_load: Cargo = None | ||
) -> None: | ||
super().__init__(name, weight, coords) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, avoid using mutable default arguments. And also, you should avoid using mutable objects (like lists) for default arguments, because they are created at function definition, not at function call. So if you modify the default argument, it will be modified for all future function calls as well.
app/main.py
Outdated
def unhook_load(self) -> str: | ||
if self.current_load is None: | ||
raise ValueError("Cannot unhook load. There is no cargo hooked.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raising a ValueError
seems a bit harsh here. You might want to consider simply returning an error message instead. It would make your program more robust and less likely to crash due to unhandled exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on the code! There are no critical issues. However, there are a few areas that could be improved for better code quality. In the hook_load
and unhook_load
methods, consider raising exceptions in case of errors instead of returning string messages. Also, there is some code duplication in the methods go_forward
, go_back
, go_right
, and go_left
and in the initialization of coords
. This could be generalized to reduce redundancy. Keep up the good work and always strive for continuous improvement. 👍🏼
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
self, name: str, weight: int, coords: list[int] = None | ||
) -> None: | ||
if coords is None: | ||
coords = [0, 0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code duplication: The initialization of coords
is duplicated in the BaseRobot
, FlyingRobot
, and DeliveryDrone
classes. You can move this logic to the BaseRobot
class and remove it from the other classes.
def go_forward(self, step: int = 1) -> list[int]: | ||
self.coords[1] += step | ||
return self.coords | ||
|
||
def go_back(self, step: int = 1) -> list[int]: | ||
self.coords[1] -= step | ||
return self.coords | ||
|
||
def go_right(self, step: int = 1) -> list[int]: | ||
self.coords[0] += step | ||
return self.coords | ||
|
||
def go_left(self, step: int = 1) -> list[int]: | ||
self.coords[0] -= step | ||
return self.coords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code duplication: The methods go_forward
, go_back
, go_right
, and go_left
are identical except for the index and operation. You could generalize this into a single method that takes the index and operation as parameters.
def hook_load(self, cargo: Cargo) -> str: | ||
if self.current_load is not None: | ||
return "Cannot hook load. There is already cargo hooked." | ||
elif cargo.weight > self.max_load_weight: | ||
return ( | ||
f"Cannot hook load. The cargo is too heavy: " | ||
f"{cargo.weight} > {self.max_load_weight}." | ||
) | ||
self.current_load = cargo | ||
return f"Cargo weighing {cargo.weight} hooked successfully." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code quality: The hook_load
method returns a string message. It would be better if it raises exceptions in case of errors. This way, the caller of the method can decide how to handle these exceptions, which is more flexible and idiomatic.
def unhook_load(self) -> str: | ||
if self.current_load is None: | ||
return "No cargo to unhook." | ||
|
||
unhooked_cargo = self.current_load | ||
self.current_load = None | ||
return f"Cargo weighing {unhooked_cargo.weight} unhooked successfully." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code quality: The unhook_load
method also returns a string message. Similar to hook_load
, it would be better to raise an exception in case of an error.
No description provided.