Solution#2937
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a couple of critical checklist violations and a test-breaking side effect that must be addressed before this can be considered complete.
Critical issues to fix
- Your
create_person_listfunction currently builds the list ofPersoninstances using a manual loop withresult = []andappend. This conflicts with the requirement to use a list comprehension when creating a list of instances from a collection [CHECKLIST ITEM #3]. Refactor that first loop to something likeresult = [Person(p["name"], p["age"]) for p in people_data](adjust names to your code) so the list is constructed in a single comprehension. - The spouse checks use explicit key membership and indexing, e.g.
if "wife" in subject and subject["wife"] is not Noneand similarly forhusband. This pattern is explicitly called out as a bad example in the checklist [CHECKLIST ITEM #4]. Replace these withif subject.get("wife") is not None:andelif subject.get("husband") is not None:so you rely ondict.get()instead ofin+ indexing. - Your module currently instantiates
Personobjects at the top level (e.g. creating sample people whenapp/main.pyis imported). This causesPerson.peopleto be non-empty as soon as the module loads, which breaks the tests that expectPerson.peopleto be empty initially. Remove or move those top-level instance creations into a separate function or test setup so that importing the module only defines the class and functions, without side effects.
Once you address these points, your solution will still have the correct behavior while also satisfying the style checklist and passing the tests. The core logic of Person and the spouse linking is solid, so you’re very close; these changes will align your code fully with the project’s requirements.
✨ 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
| def create_person_list(people: list) -> list: | ||
| # write your code here | ||
| pass | ||
| result = [] |
There was a problem hiding this comment.
This 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 building the list of Person instances using a comprehension instead of initializing result = [] and appending in a loop.
| # write your code here | ||
| pass | ||
| result = [] | ||
| for subject in people: |
There was a problem hiding this comment.
This loop for building result also falls under checklist item #3 about preferring list comprehensions for creating lists of instances from collections.
| result.append(person) | ||
|
|
||
| for subject in people: | ||
| if "wife" in subject and subject["wife"] is not None: |
There was a problem hiding this comment.
This violates checklist item #4: "Bad example: if "wife" in person and person["wife"]:". Your condition if "wife" in subject and subject["wife"] is not None matches the bad pattern; use subject.get("wife") in the condition instead of explicit key checks.
| if "wife" in subject and subject["wife"] is not None: | ||
| current_person = Person.people[subject["name"]] | ||
| current_person.wife = Person.people[subject["wife"]] | ||
| elif "husband" in subject and subject["husband"] is not None: |
There was a problem hiding this comment.
This violates checklist item #4 as well: elif "husband" in subject and subject["husband"] is not None uses explicit key membership and indexing instead of subject.get("husband"). Switch to .get() to align with the guideline.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still one critical requirement not met in app/main.py: importing the module must not create any Person instances or populate Person.people. Your Person class and the create_person_list function themselves look correct and align with the task: instances are stored in Person.people, the list comprehension is used properly, and spouses are linked by reference via names.
However, the if __name__ == "__main__": block at the bottom of app/main.py (around the last lines of the file) creates example data and calls create_person_list, which in turn creates Person objects and fills Person.people. Even though this is guarded by the __main__ check, the project requirement explicitly states that this module should only define the class and function and that tests expect Person.people to be empty immediately after import. To fix this, remove that entire if __name__ == "__main__": block from app/main.py, or move the example code into a separate script or into your tests.
Once that block is gone, the module will satisfy the requirement of having no side effects on import while preserving the correct core logic. Overall, your main implementation is solid; this is just about aligning with the no-side-effects requirement so tests can run in a clean environment.
✨ 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
| return result | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
The if __name__ == "__main__": block that creates example data and calls create_person_list causes Person instances to be created when this module is executed directly; per the previous high-priority requirement, top-level Person instantiations should be removed or moved elsewhere so that importing this module has no side effects and Person.people starts empty.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it correctly meets the functional requirements: the Person class is implemented with the right __init__, instances are registered in the Person.people dictionary, and create_person_list returns a proper list of Person objects with spouse relationships set only when the input value is not None. You handled the spouse linking well by using the shared Person.people registry so that wife and husband refer to the exact same Person instances, which matches the expected behavior.
You also did a good job following the style checklist: variable names are clear and descriptive, you used a list comprehension to build the list of people [CHECKLIST ITEM #3], and you used dict.get() to safely access optional keys [CHECKLIST ITEM #4]. Another solid improvement is that there is no longer any executable code at import time, so the module now only defines the class and function, which is good practice for reusable code. Overall, your solution is clean, readable, and aligned with the task’s requirements—well done, keep building on this structure in future exercises.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.