Solved the task! #2331
Solved the task! #2331
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the core logic correctly! However, I'm requesting a few changes to align your code with the project's specific style guidelines. Please address the following points:
- List Comprehension: The
forloop used to create the list ofPersoninstances should be converted to a list comprehension as required by [CHECKLIST ITEM #3]. - Dictionary Access: Instead of checking for keys with
if "wife" in d, please use thedict.get()method, likeif d.get("wife"):. This is the preferred approach according to [CHECKLIST ITEM #4]. - Remove Comments: The comments on lines 8, 12, 17, and 21 should be removed, as the guidelines require the code to be self-explanatory.
After these adjustments, 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
| def __init__(self, name: str, age: int) -> None: | ||
| self.name = name | ||
| self.age = age | ||
| Person.people[name] = self # регистрируем в реестре |
There was a problem hiding this comment.
According to the task guidelines, comments should be avoided as the code should be self-explanatory. Please remove the comments on lines 8, 12, 17, and 21.
app/main.py
Outdated
| for d in people: | ||
| instances.append(Person(d["name"], d["age"])) |
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.' This block can be rewritten as a single list comprehension.
app/main.py
Outdated
| # Проход 2: проставляем ссылки супругов (атрибут создаём только если значение не None) | ||
| for d in people: | ||
| p = Person.people[d["name"]] | ||
| if "wife" in d and d["wife"] is not None: |
There was a problem hiding this comment.
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 condition by using if d.get('wife'):.
app/main.py
Outdated
| p = Person.people[d["name"]] | ||
| if "wife" in d and d["wife"] is not None: | ||
| p.wife = Person.people[d["wife"]] # type: ignore[attr-defined] | ||
| if "husband" in d and d["husband"] is not None: |
There was a problem hiding this comment.
Similar to the 'wife' check, this condition violates checklist item #4. It can be simplified by using d.get('husband').
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 task! You've addressed almost all the feedback and the solution is very close to perfect. However, one final change is needed to meet all the project's style requirements. The current implementation checks for dictionary keys using the in operator (e.g., if "wife" in d:). As per the checklist, you should use the dict.get() method instead. You can update the conditions to if d.get("wife"): and if d.get("husband"):. This works because .get() returns None if the key is missing, and None is treated as False in a conditional check. This change is required to comply with [CHECKLIST ITEM #4]. Once you make this small adjustment, the code 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
|
|
||
| for d in people: | ||
| p = Person.people[d["name"]] | ||
| if "wife" in d and d["wife"] is not None: |
There was a problem hiding this comment.
This check still violates checklist item #4, which requires using the dict.get() method for accessing dictionary values. You can simplify this by using if d.get("wife"):.
app/main.py
Outdated
| p = Person.people[d["name"]] | ||
| if "wife" in d and d["wife"] is not None: | ||
| p.wife = Person.people[d["wife"]] | ||
| if "husband" in d and d["husband"] is not None: |
There was a problem hiding this comment.
Similar to the check for 'wife', this should be updated to use d.get("husband") to comply with checklist item #4.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work! I am approving this solution. You've successfully addressed the feedback from the previous review, and your use of dict.get() is now perfect [CHECKLIST ITEM #4]. The code is clean, correct, and meets all the requirements of the task. 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.