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 #1650

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

Solution #1650

wants to merge 2 commits into from

Conversation

Bogdannna7
Copy link

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.

Thank you for your hard work on this pull request! While your code is mostly on the right track, there are a few critical issues that need to be addressed before we can approve it. These issues could impact the functionality and reliability of your code:

  1. Typographical Error: There's a typo in the method name add_partner, where the parameter is named pertner_name instead of partner_name. This needs to be corrected to ensure clarity and prevent potential errors.

  2. Dictionary Access: The current approach of accessing dictionary values using list(people[index].values())[2] is not reliable. It assumes that the partner's name is always the third value, which might not hold true if the dictionary structure changes. It's better to use a key to access the partner's name directly to avoid potential issues.

  3. Use of setattr: Using setattr with list(people[index].keys())[2] assumes that the partner attribute is always the third key. This approach might not be reliable if the dictionary keys are not ordered or if the structure changes. Consider setting the partner attribute directly using a key.

Please address these issues to ensure your code is robust and meets the task requirements. Keep up the great work, and don't hesitate to reach out if you have any questions! 😊

✨ 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 Show resolved Hide resolved
app/main.py Outdated
Person.people.update({self.name: self})

def add_partner(self, pertner_name: str) -> None:
self.partner = Person.people[pertner_name]

Choose a reason for hiding this comment

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

There is a typo in the variable name pertner_name. It should be partner_name.

app/main.py Outdated Show resolved Hide resolved
app/main.py Outdated
for index, obj in enumerate(people_list):
partner_name = list(people[index].values())[2]
if partner_name is not None:
partner = Person.people[list(people[index].values())[2]]

Choose a reason for hiding this comment

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

Similar to line 17, accessing the partner's name using list(people[index].values())[2] is not reliable. Consider using a key to access the partner's name directly.

app/main.py Outdated 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 submission! 🎉 Your code in app/main.py meets most of the task requirements, and it's clear you've put in a lot of effort. However, there are a few areas where improvements can be made:

  1. Dictionary Access: To prevent KeyError exceptions, consider using the dict.get() method for safer dictionary key access.

  2. List Comprehension: While your current use of a for loop works, using list comprehension can make your code more concise and efficient.

  3. Use of setattr and delattr: Directly setting attributes without using setattr and delattr can simplify your code and reduce potential errors.

  4. Partner Attribute Handling: Ensure that the partner's name exists in Person.people before accessing it to avoid KeyError exceptions.

These adjustments will enhance the robustness and readability of your code. Remember, addressing these issues will not only improve this project but also strengthen your coding skills for future tasks. Keep up the great work, and continue learning from each experience! 🚀

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

self.age = age
Person.people.update({self.name: self})

def add_partner(self, partner_name: str) -> None:

Choose a reason for hiding this comment

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

The add_partner method is correctly defined here, but ensure that the partner's name exists in Person.people before accessing it to avoid potential KeyError exceptions.

Comment on lines +20 to +24
if person["wife"] is not None:
new_obj.wife = person["wife"]
except (KeyError, AttributeError):
if person["husband"] is not None:
new_obj.husband = person["husband"]

Choose a reason for hiding this comment

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

The current logic assumes that a person can have either a 'wife' or a 'husband' attribute. However, if both keys are missing, this will raise a KeyError. Consider using get method to safely access dictionary keys without raising exceptions.

Comment on lines +28 to +35
partner = Person.people[obj.wife]
delattr(obj, "wife")
setattr(obj, "wife", partner)
except (KeyError, AttributeError):
try:
partner = Person.people[obj.husband]
delattr(obj, "husband")
setattr(obj, "husband", partner)

Choose a reason for hiding this comment

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

The use of delattr and setattr is unnecessary here. You can directly assign the partner object to the attribute without deleting it first. This will simplify the code and reduce potential errors.

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.

None yet

2 participants