py-person-class #2333
py-person-class #2333
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is functionally correct and meets most of the requirements, which is great work! However, a couple of changes are needed to align with the project's coding standards.
The implementation currently uses a for loop to create the Person instances. This should be refactored into a list comprehension as required by the checklist [CHECKLIST ITEM #3]. Additionally, the variable i used in the loops is not descriptive. Please rename it to something more meaningful, like person_data, to improve readability [CHECKLIST ITEM #1].
Once these adjustments are made, your solution will be ready for approval.
✨ 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
app/main.py
Outdated
| # write your code here | ||
| pass | ||
| Person.people.clear() | ||
| for i in people: |
There was a problem hiding this comment.
This violates checklist item #1: 'Use descriptive and correct variable names.' The variable i is not descriptive of the data it holds (a dictionary representing a person). A name like person_data or person_dict would be more meaningful. This feedback also applies to the loop on line 18.
app/main.py
Outdated
| for i in people: | ||
| name = i["name"] | ||
| age = i["age"] | ||
| Person(name, age) |
There was a problem hiding this comment.
This loop violates checklist item #3, which states: '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.' You can replace this entire loop with a single list comprehension to create the Person objects.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this iteration! I am approving your solution. You've successfully addressed all the previous feedback by refactoring the loop into a clean list comprehension and using more descriptive variable names. The code is now much more readable and fully compliant with all project requirements. Well done!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.