'Solution_v1.0' #1239
'Solution_v1.0' #1239
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! While your implementation meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Class vs. Instance Attribute: The use of
Cafe.count_friendsas a class attribute is problematic, especially when resetting it within the function. This can lead to issues if multiple instances ofCafeare used concurrently. Consider refactoring this to use an instance attribute instead. -
Return Type Hint: The return type hint for the
visit_cafemethod should be adjusted. Currently, it only accounts forVaccineError, but the method can raise multiple types of exceptions. Consider usingstr | Exceptionor listing all specific exceptions that can be raised. -
Initialization of
count_friends: Thecount_friendsattribute is initialized toNone. If it's intended to be a counter, initializing it to0would be more appropriate. -
Error Messages: Ensure that exceptions raised in the
visit_cafemethod have descriptive error messages to improve clarity and debugging.
Please address these issues, and feel free to reach out if you have any questions. You're doing great, and these changes will help improve the robustness of your code! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
app/cafe.py
Outdated
|
|
||
|
|
||
| class Cafe: | ||
| count_friends = None |
There was a problem hiding this comment.
The count_friends attribute is initialized to None. If this is intended to be a counter, it might be better to initialize it to 0.
app/cafe.py
Outdated
| def __init__(self, name: str) -> None: | ||
| self.name = name | ||
|
|
||
| def visit_cafe(self, visitor: dict) -> str | VaccineError: |
There was a problem hiding this comment.
The return type hint str | VaccineError is incorrect because the method can raise multiple types of errors, not just VaccineError. Consider using str | Exception or listing all specific exceptions that can be raised.
app/main.py
Outdated
|
|
||
|
|
||
| def go_to_cafe(friends: list, cafe: Cafe) -> str: | ||
| Cafe.count_friends = 0 |
There was a problem hiding this comment.
Using Cafe.count_friends as a class attribute and resetting it here can lead to issues if multiple instances of Cafe are used concurrently. Consider using an instance attribute instead.
app/main.py
Outdated
| except (OutdatedVaccineError, NotVaccinatedError): | ||
| return "All friends should be vaccinated" | ||
| except NotWearingMaskError: | ||
| Cafe.count_friends += 1 |
There was a problem hiding this comment.
The logic here seems to be incorrect. If a friend is not wearing a mask, you are incrementing Cafe.count_friends, but there is no return statement, which means the loop will continue. This might not be the intended behavior.
app/main.py
Outdated
| Cafe.count_friends += 1 | ||
|
|
||
| if Cafe.count_friends > 0: | ||
| return f"Friends should buy {cafe.count_friends} masks" |
There was a problem hiding this comment.
The use of cafe.count_friends here is incorrect. It should be Cafe.count_friends if you intend to use the class attribute. However, it's better to refactor this to use an instance attribute as mentioned earlier.
There was a problem hiding this comment.
Your code meets the task requirements and constraints, and the project structure is well-organized. However, there are some areas for improvement:
-
Descriptive Error Messages: The error messages in the
visit_cafemethod should be more descriptive. Instead of using the exception names like"NotVaccinatedError", consider messages like"Visitor is not vaccinated"to provide better clarity. -
Function Logic: In the
go_to_cafefunction, returning a message inside the loop can cause premature termination. Ensure all friends are checked by using a flag or collecting results before returning a message.
These improvements will enhance the clarity and functionality of your code. Keep up the good work, and continue to refine your skills by addressing these issues in future projects. Your progress is important, and learning from these observations will help you grow as a developer. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
app/cafe.py
Outdated
| def visit_cafe(self, visitor: dict) -> str: | ||
| current_date = datetime.date.today() | ||
| if "vaccine" not in visitor: | ||
| raise NotVaccinatedError("NotVaccinatedError") |
There was a problem hiding this comment.
The error message 'NotVaccinatedError' is not descriptive. According to the checklist, you should provide a more descriptive error message when raising exceptions. Consider including more context in the message, such as 'Visitor is not vaccinated.'
app/cafe.py
Outdated
| if "vaccine" not in visitor: | ||
| raise NotVaccinatedError("NotVaccinatedError") | ||
| if visitor["vaccine"]["expiration_date"] < current_date: | ||
| raise OutdatedVaccineError("OutdatedVaccineError") |
There was a problem hiding this comment.
The error message 'OutdatedVaccineError' is not descriptive. As per the checklist, provide a more descriptive error message. You could use something like 'Visitor's vaccine is outdated.'
app/cafe.py
Outdated
| if visitor["vaccine"]["expiration_date"] < current_date: | ||
| raise OutdatedVaccineError("OutdatedVaccineError") | ||
| if visitor["wearing_a_mask"] is False: | ||
| raise NotWearingMaskError("NotWearingMaskError") |
There was a problem hiding this comment.
The error message 'NotWearingMaskError' is not descriptive. The checklist suggests using descriptive error messages. Consider using a message like 'Visitor is not wearing a mask.'
app/main.py
Outdated
| try: | ||
| cafe.visit_cafe(friend) | ||
| except (OutdatedVaccineError, NotVaccinatedError): | ||
| return "All friends should be vaccinated" |
There was a problem hiding this comment.
Returning 'All friends should be vaccinated' inside the loop will cause the function to exit on the first friend who is not vaccinated or has an outdated vaccine. This means not all friends are checked. Consider using a flag or collecting results to ensure all friends are processed before returning a message.
No description provided.