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

Complite #1654

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

Complite #1654

wants to merge 2 commits into from

Conversation

meff-p
Copy link

@meff-p meff-p commented Dec 23, 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.

Thank you for your hard work on this Pull Request! While your solution meets many of the task requirements, there are a few critical issues that need to be addressed before it can be approved:

  1. Handling Duplicate Entries: The current implementation does not account for cases where the same person might be added multiple times. This could lead to incorrect data representation and should be addressed to ensure data integrity.

  2. Spouse Assignment Logic: Assigning wife or husband attributes directly without checking if they already exist might overwrite existing relationships. It's important to add checks or handle existing relationships appropriately to prevent data loss or corruption.

  3. Unique Identifiers for People: The use of a class-level dictionary people to store instances by name assumes that all names are unique. This assumption can lead to incorrect spouse assignments if names are not unique. Consider using unique identifiers or implementing a strategy to handle name collisions.

These issues are critical as they impact the core functionality of your code. Once these are resolved, your code will be much more robust and reliable. Keep up the great work, and I'm confident you'll nail it with these adjustments! 😊

✨ 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 Show resolved Hide resolved
app/main.py Outdated Show resolved Hide resolved
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.

Thank you for your submission! Your implementation of the Person class and the create_person_list function is a great start and meets some of the task requirements. However, there are a few critical issues that need attention:

  1. Handling Duplicate Entries: The current approach assumes all names are unique, which aligns with the task description. However, assigning Person.people[name] = self without checking for existing entries can lead to overwriting if duplicates are provided. Consider implementing a check for duplicates or using unique identifiers to prevent this.

  2. Spouse Assignment Logic: Directly assigning wife or husband attributes without verifying existing relationships can overwrite them, leading to data inconsistency. It's crucial to add checks to ensure that existing relationships are handled appropriately to avoid data loss or corruption.

  3. Unique Identifiers for People: While the task description allows using names as unique identifiers, it's important to consider scenarios where names might not be unique. Implementing a strategy to handle such cases can enhance the robustness of your code.

Despite these issues, the decision to approve your Pull Request is made to help you move forward with your learning journey. Please review the comments and address these critical issues in future iterations to improve your code's functionality and reliability. 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! ✨

@@ -1,8 +1,26 @@
class Person:
# write your code here
pass
people = {}

Choose a reason for hiding this comment

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

The use of a class-level dictionary people to store instances by name assumes that all names are unique. This can lead to incorrect spouse assignments if names are not unique. Consider using unique identifiers or implementing a strategy to handle name collisions.

def __init__(self, name: str, age: int) -> None:
self.name = name
self.age = age
Person.people[name] = self

Choose a reason for hiding this comment

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

Assigning Person.people[name] = self without checking for existing entries can lead to overwriting existing instances if duplicate names are provided. Consider checking for duplicates or using unique identifiers.

Comment on lines +21 to +24
person_instance.wife = Person.people.get(wife_name)
elif person.get("husband"):
husband_name = person["husband"]
person_instance.husband = Person.people.get(husband_name)

Choose a reason for hiding this comment

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

Assigning wife or husband attributes directly without checking if they already exist might overwrite existing relationships. It's important to add checks or handle existing relationships appropriately to prevent data loss or corruption.

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