Skip to content

feat: propose py person class approach#2944

Open
ht1204 wants to merge 1 commit into
mate-academy:masterfrom
ht1204:develop
Open

feat: propose py person class approach#2944
ht1204 wants to merge 1 commit into
mate-academy:masterfrom
ht1204:develop

Conversation

@ht1204

@ht1204 ht1204 commented Jul 4, 2026

Copy link
Copy Markdown

Description

  • Proposal py person class

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

Your solution is approved because it correctly implements all required functionality: the Person class, the people class attribute, instance registration in __init__, and the create_person_list function that builds and links wife/husband relationships behave as specified. The example scenario with Ross, Rachel, and Joey would work as intended, including the AttributeError when accessing wife on a person who has no spouse set.

There are a couple of style issues to be aware of for future tasks. First, the create_person_list(people: list) parameter name is confusing because it shadows Person.people; renaming it to something like people_data or person_dicts would better satisfy [CHECKLIST ITEM #1] on descriptive variable names. Second, when you create the instances, you still use person_dict["name"] and person_dict["age"]; switching these to person_dict.get("name") and person_dict.get("age") would align fully with [CHECKLIST ITEM #4] about preferring dict.get() for dictionary access. Overall, your core implementation is solid; just keep refining variable naming and dictionary access style to match the guidelines more closely in future exercises.


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

Footnotes

  1. Rate AI review example

Comment thread app/main.py
Person.people[name] = self


def create_person_list(people: list) -> list:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This violates checklist item #1: Use descriptive and correct variable names. Using people here shadows Person.people and makes it less clear that this is a list of dictionaries, not the class-level map of instances.

Comment thread app/main.py
# write your code here
pass
person_instances = [
Person(person_dict["name"], person_dict["age"])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This violates checklist item #4: When accessing values in a dictionary, it is better to use the dict.get() method instead of explicitly checking for the presence of a key. You are directly indexing the dictionary for "name" and "age"; consider using person_dict.get("name") and person_dict.get("age") to align with the guideline.

Comment thread app/main.py
pass
person_instances = [
Person(person_dict["name"], person_dict["age"])
for person_dict in people

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This violates checklist item #1: Use descriptive and correct variable names. Using people as a parameter name makes it ambiguous and conflicts conceptually with Person.people (the class attribute). A more descriptive name like people_data would better convey that this is input data.

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