Skip to content

Solution#2959

Open
Rimbozero wants to merge 2 commits into
mate-academy:masterfrom
Rimbozero:master
Open

Solution#2959
Rimbozero wants to merge 2 commits into
mate-academy:masterfrom
Rimbozero:master

Conversation

@Rimbozero

Copy link
Copy Markdown

No description provided.

@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

I am requesting changes because, while your implementation is functionally correct, it does not yet meet all of the explicit checklist requirements, which are treated as critical for this task. The creation of person_list in app/main.py currently uses a traditional for loop instead of a list comprehension, violating [CHECKLIST ITEM #3]; refactor it to something like [Person(p["name"], p["age"]) for p in people] so all instances are created in a single expression. For spouse linking, your conditions use explicit key checks such as "wife" in person_dict and person_dict["wife"] is not None, which matches the “bad example” in [CHECKLIST ITEM #4]; instead, use person_dict.get("wife") (and similarly person_dict.get("husband")) directly in the if condition to simplify and align with the required style. The inline comments around create_person_list logic are also unnecessary given the clarity of the code and conflict with the guideline that comments are not needed for self-explanatory code; please remove those to keep the implementation clean. Once you address these style-based checklist violations, your otherwise solid and correct solution will fully satisfy both the functional and stylistic requirements—nice work on getting the spouse linking and Person.people registry behavior right so far.


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

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread app/main.py Outdated
Comment on lines +16 to +17
for person in people:
person_list.append(Person(person["name"], person["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 loop that builds person_list violates checklist item #3: "When creating a list of instances from a collection (such as a list of dictionaries), it is recommended to use list comprehension rather than traditional for loops." Consider replacing the loop with a list comprehension that directly constructs all Person instances.

Comment thread app/main.py Outdated
for person_dict in people:
person = Person.people[person_dict["name"]]

if "wife" in person_dict and person_dict["wife"] is not None:

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 dictionary access pattern matches the "Bad example" in checklist item #4: if "wife" in person and person["wife"]. You should instead use person_dict.get("wife") in the condition to follow the recommended style and avoid explicit key checks.

Comment thread app/main.py Outdated
if "wife" in person_dict and person_dict["wife"] is not None:
person.wife = Person.people[person_dict["wife"]]

if "husband" in person_dict and person_dict["husband"] is not None:

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 dictionary access pattern similarly matches the "Bad example" from checklist item #4 for checking husband; switch to using person_dict.get("husband") in the condition instead of checking key membership explicitly.

Comment thread app/main.py Outdated
Comment on lines +15 to +19
# First create all Person instances
for person in people:
person_list.append(Person(person["name"], person["age"]))

# Then add wife/husband links

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The checklist note says "There is no need to add comments to the code as it is clear and self-explanatory." These comments are not needed and can be removed to align with the clean code guideline.

@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

I am approving your solution because it correctly implements all the required functionality and matches the specified behaviors. Your Person class has the proper __init__(self, name, age) signature, maintains a class-level people dictionary, and registers each instance by its name. The create_person_list function rebuilds Person.people, uses a list comprehension to create all Person instances, and then sets wife/husband attributes only when the corresponding dict value is present, correctly linking to the existing instances. This design ensures that cases like person_list[0].wife is person_list[2] and mutual spouse references work as intended, while absent spouse data does not create attributes, leading to the expected AttributeError when accessed. You’ve also followed the checklist preferences by using dict.get() instead of manual key checks, avoiding nested if statements, and keeping the code clear and free of unnecessary comments. Overall, your implementation is clean, readable, and fully aligned with the task requirements.


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

Footnotes

  1. Rate AI review example

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