-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
'Solution' #1226
base: master
Are you sure you want to change the base?
'Solution' #1226
Conversation
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.
Thank you for your hard work on this Pull Request! 😊 There are a couple of critical issues that need to be addressed before we can approve it. Firstly, the immediate return upon catching a VaccineError
in app/main.py
can prevent the function from checking all friends, which impacts the functionality. Secondly, the redundancy in exception handling for VaccineError
and its subclasses in app/cafe.py
should be simplified to improve code clarity. Please make these changes, and feel free to reach out if you need any help! Keep up the great work! 🚀
✨ 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
except VaccineError: | ||
raise | ||
except NotWearingMaskError: | ||
raise |
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.
The exception handling for VaccineError
is redundant because VaccineError
is a superclass of NotVaccinatedError
and OutdatedVaccineError
. Since these specific exceptions are already being raised, catching VaccineError
separately is unnecessary. You can remove the except VaccineError
block to simplify the code.
app/main.py
Outdated
except VaccineError: | ||
return "All friends should be vaccinated" |
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.
Returning immediately upon catching a VaccineError
means that if any friend is not vaccinated, the function will exit without checking the rest of the friends. Consider accumulating results or flags for all friends before deciding on the return value, to ensure all friends are checked.
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.
Great job on implementing the code! 🎉 While there are some areas for improvement, such as enhancing error messages and refining exception handling, your effort is commendable. Please review the comments to enhance your code further. Addressing these issues will help you avoid similar problems in the future and improve your coding skills. Keep up the good work and continue learning from these experiences!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
def visit_cafe(self, visitor: dict) -> str: | ||
try: | ||
if "vaccine" not in visitor: | ||
raise NotVaccinatedError("NotVaccinatedError") |
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.
The error message 'NotVaccinatedError' is not informative. Consider providing a more descriptive message that explains why the error is being raised.
if "vaccine" not in visitor: | ||
raise NotVaccinatedError("NotVaccinatedError") | ||
elif visitor["vaccine"]["expiration_date"] < datetime.date.today(): | ||
raise OutdatedVaccineError("OutdatedVaccineError") |
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.
The error message 'OutdatedVaccineError' is not informative. Consider providing a more descriptive message that explains why the error is being raised.
elif visitor["vaccine"]["expiration_date"] < datetime.date.today(): | ||
raise OutdatedVaccineError("OutdatedVaccineError") | ||
elif not visitor["wearing_a_mask"]: | ||
raise NotWearingMaskError("NotWearingMaskError") |
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.
The error message 'NotWearingMaskError' is not informative. Consider providing a more descriptive message that explains why the error is being raised.
raise NotWearingMaskError("NotWearingMaskError") | ||
return f"Welcome to {self.name}" | ||
except NotWearingMaskError: | ||
raise |
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.
The 'raise' statement here will re-raise the 'NotWearingMaskError' without any additional handling or message. Consider adding a custom message or handling logic if needed.
for friend in friends: | ||
try: | ||
cafe.visit_cafe(friend) | ||
except VaccineError: |
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.
Catching 'VaccineError' will handle both 'NotVaccinatedError' and 'OutdatedVaccineError'. If you want to differentiate between these two cases, catch them separately.
masks_to_buy += 1 | ||
|
||
if not_vaccinated: | ||
return "All friends should be vaccinated" |
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.
The message 'All friends should be vaccinated' might be misleading if only some friends are not vaccinated. Consider rephrasing it to indicate the actual number of friends who are not vaccinated.
No description provided.