-
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
Solution #772
base: master
Are you sure you want to change the base?
Solution #772
Conversation
app/main.py
Outdated
def add_to_people(self) -> None: | ||
Person.people[self.name] = self |
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.
maybe it's better to add person to people dictionary inside init method to make sure person is only added to dictionary when new instance is created.
app/main.py
Outdated
people_list = [] | ||
|
||
for person_info in people: | ||
name = person_info["name"] | ||
age = person_info["age"] | ||
person = Person(name, age) | ||
people_list.append(person) |
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.
maybe you can try doing it with list comprehensions
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.
few improvements can be implemented, but looks good overall
app/main.py
Outdated
self.name = name | ||
self.age = age | ||
self.spouse = None | ||
self.add_to_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.
Create class Person. It's __init__ method takes and store name, age of a person. This class also should have a class attribute people, it is a dict that stores Person instances by their name
-- only 3 attributes according to the task description
app/main.py
Outdated
def add_to_people(self) -> None: | ||
Person.people[self.name] = self |
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.
You can do this directrly in the init
app/main.py
Outdated
people_list = [] | ||
|
||
for person_info in people: | ||
name = person_info["name"] | ||
age = person_info["age"] | ||
person = Person(name, age) | ||
people_list.append(person) |
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.
Try to rewrite in one line. Hint: list comprehension 🤫
app/main.py
Outdated
person = Person(name, age) | ||
people_list.append(person) | ||
|
||
for index, person_info in enumerate(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.
Use just dict.get() method in the condition, instead of this long one
app/main.py
Outdated
for index, person_info in enumerate(people): | ||
if "wife" in person_info and person_info["wife"]: | ||
wife_name = person_info["wife"] | ||
if wife_name in Person.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.
This check does nothing here, you can remove it
app/main.py
Outdated
Person.people[name] = self | ||
|
||
@classmethod | ||
def couple_create(cls, name: str, couple_name: str, is_male: bool) -> None: |
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.
its not couple_name, its partner name
app/main.py
Outdated
if person.get("wife"): | ||
Person.couple_create(person["name"], person["wife"], True) | ||
elif person.get("husband"): | ||
Person.couple_create(person["name"], person["husband"], False) |
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.
You make the same cheks twice: here and in the couple_create method
Try to rewrite without creating additional method, it decrease code amount and increase readability
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.
fix that))
app/main.py
Outdated
if person.get("wife"): | ||
Person.couple_create(person["name"], person["wife"], True) | ||
elif person.get("husband"): | ||
Person.couple_create(person["name"], person["husband"], False) |
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.
fix that))
app/main.py
Outdated
Person.people[name] = self | ||
|
||
@classmethod | ||
def couple_create(cls, |
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.
fix that was said earlier) or text me in direct messages in Slack if u dont uderstand my advice)
app/main.py
Outdated
for person_data in people: | ||
name = person_data["name"] | ||
age = person_data["age"] | ||
new_person = Person(name, age) | ||
list_of_people.append(new_person) |
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.
list comprehention
app/main.py
Outdated
person = Person.people[person_data["name"]] | ||
person.wife = Person.people[person_data["wife"]] | ||
Person.people[person_data["wife"]].husband = person | ||
elif "husband" in person_data and \ |
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.
Don't use \
as a linebreaker, create temporary variable instead or use parentheses
app/main.py
Outdated
] | ||
|
||
for person_data in people: | ||
if "wife" in person_data and person_data["wife"] in Person.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.
Use just dict.get() method in the condition, instead of this long one
app/main.py
Outdated
if "wife" in person_data and person_data["wife"] in Person.people: | ||
person = Person.people[person_data["name"]] | ||
person.wife = Person.people[person_data["wife"]] | ||
Person.people[person_data["wife"]].husband = person |
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.
You can just use person.wife, look at previoius line
app/main.py
Outdated
wife = Person.people.get(person_data.get("wife")) | ||
if wife is not None: | ||
person.wife = wife | ||
wife.husband = person |
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.
wife = Person.people.get(person_data.get("wife")) | |
if wife is not None: | |
person.wife = wife | |
wife.husband = person | |
if person_data.get("wife"): | |
person.wife = Person.people[person_data["wife"]] |
You can do like that for wife and 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.
if person_data.get("wife"):
# check if there is a wife for person in person_data dict
person.wife = Person.people[person_data["wife"]]
# assign to Person instance new wife attribute with wifes Person instance
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.
finally well done ^_^
No description provided.