Solution #1254
Solution #1254
Conversation
| coords: Optional[list[int]] = None) -> None: | ||
| self.name = name | ||
| self.weight = weight | ||
| self.coords = coords if coords is not None else [0, 0] |
There was a problem hiding this comment.
You can rewrite this using coords or [0, 0]
| if coords is None: | ||
| coords = [0, 0, 0] | ||
| super().__init__(name, weight, coords) |
There was a problem hiding this comment.
You don't need this condition to call super with the correct data because you do it in the parent class BaseRobot.
app/main.py
Outdated
| def __init__(self, name: str, weight: int, | ||
| coords: Optional[list[int]] = None) -> None: |
There was a problem hiding this comment.
Each argument should be written on a separate line.
app/main.py
Outdated
| def __init__(self, name: str, weight: int, | ||
| coords: Optional[list[int]] = None) -> None: |
There was a problem hiding this comment.
Each argument should be written on a separate line.
app/main.py
Outdated
| def __init__(self, name: str, weight: int, | ||
| coords: Optional[list[int]] = None, | ||
| max_load_weight: int = 0, | ||
| current_load: Optional[Cargo] = None) -> None: |
There was a problem hiding this comment.
Each argument should be written on a separate line.
There was a problem hiding this comment.
Hey there! You've done a great job with your code - it's well-structured and the logic is implemented correctly. However, there are a few improvements that could be made to enhance its functionality and user-friendliness. These include providing feedback when the cargo cannot be hooked or unhooked, and considering the use of a Tuple for coordinates for clearer understanding. Keep up the good work, and with a few tweaks, your code will be even better! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
app/main.py
Outdated
| self, | ||
| name: str, | ||
| weight: int, | ||
| coords: Optional[list[int]] = None, |
There was a problem hiding this comment.
It would be more clear to use a Tuple[int, int] for 2D coordinates and Tuple[int, int, int] for 3D coordinates instead of list[int]. It will give a strict understanding that the coords variable always contains exactly two or three integers.
app/main.py
Outdated
| if self.current_load is None and cargo.weight <= self.max_load_weight: | ||
| self.current_load = cargo |
There was a problem hiding this comment.
There is no feedback for the user in case the cargo cannot be hooked. It would be better to raise an exception or at least print a message to inform the user that the cargo is too heavy to be hooked.
| def __init__( | ||
| self, | ||
| name: str, | ||
| weight: int, |
There was a problem hiding this comment.
The weight attribute in the DeliveryDrone class seems to be unused. If it's not used for any calculations or logic, consider removing it.
app/main.py
Outdated
| def unhook_load(self) -> None: | ||
| self.current_load = None |
There was a problem hiding this comment.
The unhook_load method sets the current_load to None regardless of whether there was a load before or not. It might be a good idea to add a check if there is a load before unhooking and provide feedback to the user if there's no load to unhook.
There was a problem hiding this comment.
Great job so far! There are a few changes that need to be made before the code can be approved. These include handling default values consistently, raising exceptions instead of printing error messages, and providing more complete information in the get_info() method. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| ) -> None: | ||
| self.name = name | ||
| self.weight = weight | ||
| self.coords = coords if coords is not None else [0, 0] |
There was a problem hiding this comment.
It's a good practice to have a default value for the coordinates in the class definition, not in the init method. This way, if the coordinates are not provided during the object creation, the default value will be used.
| def go_left(self, step: int = 1) -> None: | ||
| self.coords[0] -= step | ||
|
|
||
| def get_info(self) -> str: |
There was a problem hiding this comment.
Consider including the robot's current coordinates in the get_info() method. This would provide more complete information about the robot's state.
| ) -> None: | ||
| if coords is None: | ||
| coords = [0, 0, 0] | ||
| super().__init__(name, weight, coords) |
There was a problem hiding this comment.
The way you handle the default coordinates in the FlyingRobot class is different from the BaseRobot class. For consistency, it's better to handle it in the same way in both classes. You could define a default value in the class definition.
| else: | ||
| print( | ||
| f"Error: Cargo is too heavy to be hooked " | ||
| f"(max {self.max_load_weight} kg)." |
There was a problem hiding this comment.
Instead of printing an error message directly in the hook_load() method, consider raising an exception. This would allow the calling code to handle the error in a way that is most appropriate for its needs. For example, it could log the error, display a message to the user, or take some corrective action.
| if self.current_load is not None: | ||
| self.current_load = None | ||
| else: | ||
| print("Error: No load to unhook.") |
There was a problem hiding this comment.
Similar to the previous comment, consider raising an exception in the unhook_load() method when there is no load to unhook, instead of printing an error message.
There was a problem hiding this comment.
Great job on your code! It's well-structured and easy to read. However, there are a few areas that could be improved. In particular, pay attention to how you handle errors and consider raising exceptions instead of printing error messages directly. Also, be careful with mutable data types like lists when they're used as class attributes. Keep up the good work and continue learning and improving. 🚀👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| def hook_load(self, cargo: Cargo) -> None: | ||
| if self.current_load is None: | ||
| if cargo.weight <= self.max_load_weight: | ||
| self.current_load = cargo | ||
| else: | ||
| print( | ||
| f"Error: Cargo is too heavy to be hooked " | ||
| f"(max {self.max_load_weight} kg)." | ||
| ) |
There was a problem hiding this comment.
Error handling: Instead of printing the error message directly, consider raising an exception. This will allow the caller of this method to decide how they want to handle the error. For example, you could raise a ValueError with the error message.
| def unhook_load(self) -> None: | ||
| if self.current_load is not None: | ||
| self.current_load = None | ||
| else: | ||
| print("Error: No load to unhook.") |
There was a problem hiding this comment.
Error handling: Similar to the previous comment, consider raising an exception instead of printing the error message directly. This will provide more flexibility to the caller of this method.
| ) -> None: | ||
| self.name = name | ||
| self.weight = weight | ||
| self.coords = coords if coords is not None else [0, 0] |
There was a problem hiding this comment.
Code design: The 'coords' attribute is a mutable data type (list), which is used directly from the arguments. This could potentially lead to unexpected behavior if the list passed as argument is modified outside the class. Consider creating a new list inside the init method to avoid this issue.
| coords: Optional[List[int]] = None, | ||
| ) -> None: | ||
| if coords is None: | ||
| coords = [0, 0, 0] |
There was a problem hiding this comment.
Code design: Similar to the previous comment, consider creating a new list inside the init method to avoid potential issues with mutable data types.
No description provided.