-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Ross is still Rachel's husband, and Rachel is Ross's wife. #1628
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on implementing the Person
class and the create_person_list
function! 🎉 Your code meets the task requirements and constraints effectively. While there are a few considerations regarding the use of class variables and potential KeyError
exceptions, these do not critically impact the functionality. For future improvements, you might want to handle cases where a spouse's name might not exist in the people
dictionary to avoid errors, and consider whether using a class variable for people
aligns with your design intentions. Keep up the great work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -1,8 +1,25 @@ | |||
class Person: | |||
# write your code here | |||
pass | |||
people = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The people
dictionary is a class variable, which means it will be shared across all instances of the Person
class. This might lead to unexpected behavior if you intend to have separate lists of people for different contexts. Consider whether this is the intended design.
for person_data in people: | ||
name = person_data["name"] | ||
if "wife" in person_data and person_data["wife"] is not None: | ||
Person.people[name].wife = Person.people[person_data["wife"]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code assumes that the 'wife' key in person_data
corresponds to a name that already exists in Person.people
. If the name does not exist, this will raise a KeyError
. Consider adding error handling to manage cases where the referenced person does not exist.
if "wife" in person_data and person_data["wife"] is not None: | ||
Person.people[name].wife = Person.people[person_data["wife"]] | ||
if "husband" in person_data and person_data["husband"] is not None: | ||
Person.people[name].husband = Person.people[person_data["husband"]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the handling of 'wife', the code assumes that the 'husband' key in person_data
corresponds to a name that already exists in Person.people
. This can also raise a KeyError
if the name does not exist. Consider adding error handling for this case as well.
if "husband" in person_data and person_data["husband"] is not None: | ||
Person.people[name].husband = Person.people[person_data["husband"]] | ||
|
||
return list(Person.people.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning list(Person.people.values())
will return all Person
instances ever created, due to the use of a class variable. If you only want to return the instances created in this function call, consider using a local variable to store the instances.
No description provided.