Skip to content
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 #1260

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

solution #1260

wants to merge 2 commits into from

Conversation

axe-kkk
Copy link

@axe-kkk axe-kkk commented Oct 10, 2024

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code is on the right track! However, there are some important details that need to be addressed. Make sure to add error handling for cases when the cargo is too heavy in the hook_load method. Also, consider adding comments for clarity and enhance your use of inheritance by adding an altitude attribute to the FlyingRobot class. Keep up the good work! 💪😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

app/main.py Show resolved Hide resolved
app/main.py Outdated
Comment on lines 48 to 65
class DeliveryDrone(FlyingRobot):
def __init__(self,
name: str,
weight: int,
max_load_weight: int,
current_load: None | Cargo,
coords: list = None) -> None:
super().__init__(name, weight, coords)
self.max_load_weight = max_load_weight
self.current_load = current_load

def hook_load(self, cargo: Cargo) -> None:
if (self.current_load is None
and cargo.weight <= self.max_load_weight):
self.current_load = cargo

def unhook_load(self) -> None:
self.current_load = None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the DeliveryDrone class, you're allowing for the possibility of the current_load to be None. This is a good use of Python's None type to represent the absence of a value. However, it would be helpful to add a comment here explaining why current_load can be None. This will make it easier for others (or future you) to understand your code.

app/main.py Show resolved Hide resolved
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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! 👍 However, there are a few areas that need improvement. Please consider throwing exceptions instead of printing error messages, check if the third element exists in the 'coords' list, and use tuples for storing coordinates. Keep up the good work! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

coords: list = None) -> None:
self.name = name
self.weight = weight
self.coords = coords if coords is not None else [0, 0]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good practice to use tuples for storing coordinates instead of lists. Tuples are immutable, and it's safer to use them for storing data that shouldn't be changed accidentally.

app/main.py Show resolved Hide resolved
app/main.py Show resolved Hide resolved
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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! There are a few areas that could be improved for robustness and maintainability. Be sure to pay attention to the error messages and how they are handled. It's also important to consider the data types you're using, like using a tuple instead of a list for coords. Keep up the good work and happy coding! 🚀👏

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

def __init__(self,
name: str,
weight: int,
coords: list = None) -> None:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a recommendation, consider using a tuple for coords instead of a list. Tuples are more appropriate for handling collections of heterogeneous data or records. Also, tuples are immutable, which means they can't be accidentally changed.

coords: list = None) -> None:
self.name = name
self.weight = weight
self.coords = coords if coords is not None else [0, 0]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line could potentially introduce a bug. If coords is mutable (like a list), and you use it as a default argument, it will be shared between all instances of this class. This is because default arguments in Python are evaluated once when the function is defined, not each time the function is called. You could avoid this by making coords a required argument, or by using a sentinel value to signify that no argument was provided (commonly None), and then creating a new list in the body of the function if necessary.

coords: list = None) -> None:
super().__init__(name, weight, coords)

if self.coords is not None and len(self.coords) <= 2:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're checking if self.coords is not None, but in the parent class constructor, you've already ensured that self.coords can't be None. This check is unnecessary and can be removed.

and cargo.weight <= self.max_load_weight):
self.current_load = cargo
else:
print("Drone didn't hook cargo, cargo already in current load")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of printing an error message directly to the console, consider raising an exception. This would allow the caller to decide how to handle the error (for example, they might want to log it, or display a message to the user in a GUI application). The print function should generally only be used for debugging, or for applications that are specifically intended to be run in a console.

and cargo.weight <= self.max_load_weight):
self.current_load = cargo
else:
print("Drone didn't hook cargo, cargo already in current load")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message 'Drone didn't hook cargo, cargo already in current load' is a bit misleading. It is printed not only when the drone already has a cargo, but also when the weight of the cargo exceeds max_load_weight. Consider splitting this into two separate checks with different error messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants