implement function "create_person_list" for py-person-class #1659
Conversation
There was a problem hiding this comment.
Thank you for your efforts on this task! Your code is on the right track, but there are a few critical issues that need to be addressed before it can be approved:
-
Error Handling for Missing Keys: Your code assumes that certain keys (like "wife" and "husband") exist in the
peopledictionary. This can lead toKeyErrorexceptions if the keys are missing. It's important to add error handling to manage these cases gracefully. -
Unique Names in Dictionary: The
peopledictionary uses names as keys, which means each name must be unique. Ensure that all names are unique to prevent overwriting existing entries. -
Utilization of
person_list: Theperson_listis populated but not used effectively in the function. Consider how this list can be returned or utilized within your application to enhance functionality.
Addressing these issues will improve the robustness and reliability of your code. Keep up the great work, and I'm confident you'll have this sorted out in no time! 😊
✨ 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.
| def __init__(self, name: str, age: int) -> None: | ||
| self.name = name | ||
| self.age = age | ||
| Person.people[self.name] = self |
There was a problem hiding this comment.
This line adds the Person instance to the people dictionary using the person's name as the key. Ensure that the name is unique, or previous entries will be overwritten.
| pass | ||
| person_list = [] | ||
| for person in people: | ||
| person_list.append(Person(name=person["name"], age=person["age"])) |
There was a problem hiding this comment.
The person_list is being populated with Person instances, but this list is not used later in the function. Consider returning this list or using it in some way.
| person_list.append(Person(name=person["name"], age=person["age"])) | ||
|
|
||
| for person in people: | ||
| current_person = Person.people[person["name"]] |
There was a problem hiding this comment.
Accessing Person.people[person['name']] assumes that the person already exists in the dictionary. Ensure that all names in the input list are unique and have been added to the dictionary.
app/main.py
Outdated
| current_person = Person.people[person["name"]] | ||
|
|
||
| if "wife" in person and person["wife"]: | ||
| current_person.wife = Person.people[person["wife"]] |
There was a problem hiding this comment.
This line assumes that the wife's name exists in the people dictionary. If the name does not exist, this will raise a KeyError. Consider adding error handling for missing keys.
app/main.py
Outdated
| current_person.wife = Person.people[person["wife"]] | ||
|
|
||
| if "husband" in person and person["husband"]: | ||
| current_person.husband = Person.people[person["husband"]] |
There was a problem hiding this comment.
Similar to the previous comment, this line assumes the husband's name exists in the people dictionary. Add error handling to manage cases where the name is not found.
|
|
||
| if "husband" in person and person["husband"]: | ||
| current_person.husband = Person.people[person["husband"]] | ||
| return person_list |
There was a problem hiding this comment.
The person_list is returned, but it is not clear how it is used. Ensure that the function's return value is utilized effectively in the application.
app/main.py
Outdated
| if self not in Person.people: | ||
| Person.people[self.name] = self |
There was a problem hiding this comment.
There is no need to perform this check, assign the value directly
app/main.py
Outdated
| if "wife" in person and person["wife"]: | ||
| wife_instance = Person.people.get(person["wife"]) | ||
| if wife_instance: | ||
| current_person.wife = wife_instance |
There was a problem hiding this comment.
You could avoid using nested if condition (because you've created all instances in the previous loop) and write if condition more efficiently, e.g.
| if "wife" in person and person["wife"]: | |
| wife_instance = Person.people.get(person["wife"]) | |
| if wife_instance: | |
| current_person.wife = wife_instance | |
| if wife_name := person.get("wife"): | |
| current_person.wife = Person.people[wife_name] |
There was a problem hiding this comment.
Rewrite if condition for husband as well
| # checking for the presence of the key wife or husband and if found, | ||
| # assign the value wife or husband to the instance |
There was a problem hiding this comment.
There is no need to add such comments in the future since the code is self-descriptive enough
No description provided.