-
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 #1211
base: master
Are you sure you want to change the base?
solution #1211
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.
You're doing great, but there are a few changes that need to be addressed to improve your code. Please add custom error messages to your exceptions and consider the other suggestions related to code efficiency and readability. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
def __init__(self, name: str) -> None: | ||
self.name = name | ||
|
||
def visit_cafe(self, visitor: dict) -> str: |
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.
Code Efficiency: It's not necessary to use dict.get()
method in line 14, you can directly access the dictionary key. Python will return None
if the key is not present in the dictionary. This will make your code more readable.
app/cafe.py
Outdated
if visitor_date < today: | ||
raise OutdatedVaccineError("Your vaccine is outdated!") | ||
|
||
if visitor["wearing_a_mask"] is False: |
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.
Code Efficiency: You can simply use if not visitor['wearing_a_mask']:
instead of if visitor['wearing_a_mask'] is False:
. This is more pythonic and readable.
app/errors.py
Outdated
class VaccineError(Exception): | ||
""" | ||
parent vaccine error | ||
""" | ||
|
||
|
||
class NotVaccinatedError(VaccineError): | ||
""" | ||
not vaccinated | ||
""" | ||
|
||
|
||
class OutdatedVaccineError(VaccineError): | ||
""" | ||
outdated vaccine | ||
""" | ||
|
||
|
||
class NotWearingMaskError(Exception): | ||
""" | ||
no mask | ||
""" |
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.
All your custom exceptions are missing descriptive error messages. It would be better to add a custom message to each of them. This can be done by overriding the 'init' method in each class and passing the custom message to the parent class's 'init' method. For example: 'class VaccineError(Exception): def init(self, message="Vaccine error"): super().init(message)'.
app/main.py
Outdated
@@ -1 +1,17 @@ | |||
# write your code here | |||
from app.cafe import Cafe | |||
from app.errors import VaccineError, 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.
According to the checklist, it's better to use parentheses when importing multiple modules/classes. Here is how you can do it:
python
from app.errors import (
VaccineError,
NotWearingMaskError
)
from app.errors import VaccineError, NotWearingMaskError | ||
|
||
|
||
def go_to_cafe(friends: list, cafe: Cafe) -> str: |
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 using type annotations in your function definition. This is a good practice as it improves readability and understanding of the code.
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! 👏 Your code is well-structured and follows good practices. There are a few minor issues related to code style and efficiency, but they do not significantly impact the functionality of your code. Keep up the good work and consider the given feedback for your future code. Happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
def visit_cafe(self, visitor: dict) -> str: | ||
if "vaccine" not in visitor: | ||
raise 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.
This is a Code Style issue. You are raising NotVaccinatedError
without any message. It is a good practice to provide descriptive error messages for exceptions. It will help other developers to understand the problem quickly. You might want to raise it like this: raise NotVaccinatedError('The visitor needs to be vaccinated to enter the cafe.')
visitor_date = visitor["vaccine"]["expiration_date"] | ||
today = date.today() | ||
if visitor_date < today: | ||
raise 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.
This is a Code Style issue. Similar to the previous comment, you are raising OutdatedVaccineError
without any message. It is a good practice to provide descriptive error messages for exceptions. You might want to raise it like this: raise OutdatedVaccineError('The visitor's vaccine is outdated.')
raise OutdatedVaccineError | ||
|
||
if not visitor["wearing_a_mask"]: | ||
raise 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.
This is a Code Style issue. You are raising NotWearingMaskError
without any message. It is a good practice to provide descriptive error messages for exceptions. You might want to raise it like this: raise NotWearingMaskError('The visitor is not wearing a mask.')
from app.errors import ( | ||
VaccineError, | ||
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.
Code Style: According to the checklist, it's better to use parentheses ()
when importing multiple classes/modules. Please, adjust your import statement accordingly.
) | ||
|
||
|
||
def go_to_cafe(friends: list, cafe: Cafe) -> str: |
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.
Code Style: It's a good practice to use type annotations for all function parameters and return values. For instance, cafe: Cafe
should be annotated as well.
try: | ||
cafe.visit_cafe(person) | ||
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.
Code Style: It's a good practice to provide descriptive error messages. While raising the VaccineError
, you can add a custom message to make the error more descriptive.
if masks_to_buy: | ||
return f"Friends should buy {masks_to_buy} masks" |
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.
Code Efficiency: There is no need to compare masks_to_buy
to 0
. You can just check if masks_to_buy:
as it's more efficient and cleaner.
No description provided.