-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Solution #2342
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
base: master
Are you sure you want to change the base?
Solution #2342
Conversation
mateacademy-ai-mentor
left a comment
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.
Review Summary
Good work on implementing the core logic! The two-pass approach to create and link the Person instances is correct. However, this submission requires a few changes to align with the project's coding standards as outlined in the checklist. Specifically, please address the following:
- The
forloop used to create the initial list ofPersoninstances should be replaced with a more concise list comprehension [CHECKLIST ITEM #3]. - The nested
ifstatement for checking the spouse should be flattened into a singleifstatement using a logicalandoperator [CHECKLIST ITEM #2]. - When checking for
"wife"or"husband"keys, please use thedict.get()method instead of checking for key existence within[CHECKLIST ITEM #4].
Making these adjustments will bring your code in line 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
app/main.py
Outdated
| for person_data in people: | ||
| person = Person( | ||
| name=person_data["name"], | ||
| age=person_data["age"] | ||
| ) | ||
| person_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.
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.'
This for loop can be replaced with a more concise list comprehension to create the person_list.
app/main.py
Outdated
| if spouse_key: | ||
| spouse_name = person_data.get(spouse_key) | ||
|
|
||
| if spouse_name is not 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.
This nested if statement violates checklist item #2: 'Avoid nested if by using and, or logical operators.'
The check on this line can be combined with the outer if statement on line 31 to avoid nesting.
mateacademy-ai-mentor
left a comment
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.
Review Summary
Excellent progress on this submission! You've successfully implemented the list comprehension and flattened the conditional logic as requested. There is just one remaining requirement that needs to be addressed. Your code currently uses the in operator to check for the presence of 'wife' or 'husband' keys in the dictionary. This approach violates a specific project guideline. [CHECKLIST ITEM #4] Please refactor this logic to use the dict.get() method to retrieve the value and then check if that value is not None. After that small change, this 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
| if "wife" in person_data: | ||
| spouse_key = "wife" | ||
| elif "husband" in person_data: | ||
| spouse_key = "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.
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 can simplify this logic by using dict.get() to retrieve the potential 'wife' or 'husband' value and then checking if that value is not None.
mateacademy-ai-mentor
left a comment
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.
Review Summary
I am approving this submission to help you move forward, but there's one key piece of feedback from the last review that still needs your attention. You've done a great job structuring the Person class and using list comprehension!
The main issue is that the code still uses the in operator to check for 'wife' or 'husband' keys. The requirement is to use dict.get() instead of checking for the key's presence first [CHECKLIST ITEM #4].
A good way to approach this is to try person_data.get('wife'). If that returns None (or another default value), you can then try person_data.get('husband'). This will help you avoid the in check entirely and simplify your logic. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| spouse_key = "wife" if "wife" in person_data\ | ||
| else ("husband" if "husband" in person_data else 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.
This logic still uses the in operator to check for the presence of the 'wife' and 'husband' keys, which violates checklist item #4. The requirement is to use the dict.get() method to access dictionary values without explicitly checking for key presence first.
No description provided.