-
Notifications
You must be signed in to change notification settings - Fork 612
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 #608
base: master
Are you sure you want to change the base?
Solution #608
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've done a great job, but there are a few changes that need to be made. Please, take a look at the comments and make the necessary corrections. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
app/main.py
Outdated
|
||
def __set__(self, instance: "Visitor", value: int) -> None: | ||
if not isinstance(value, int): | ||
raise TypeError() |
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.
It's a good practice to provide an error message when raising exceptions. This helps to understand what exactly went wrong. For example, you can modify this line to raise TypeError('Expected an integer value')
.
app/main.py
Outdated
if not isinstance(value, int): | ||
raise TypeError() | ||
if not (self.min_amount <= value <= self.max_amount): | ||
raise ValueError() |
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.
Same as above, it's better to provide an error message when raising exceptions. Modify this line to raise ValueError('Value out of the allowed range')
.
app/main.py
Outdated
def can_access(self, visitor: Visitor) -> bool: | ||
try: | ||
self.limitation_class(visitor.age, visitor.weight, visitor.height) | ||
except ValueError or TypeError: |
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.
When you catch multiple exceptions, they should be put into a tuple, not separated by or
. Correct this line to except (ValueError, TypeError):
.
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 this task! Your use of composition and inheritance is commendable. However, be careful not to hide exceptions in your methods. Also, consider adding some data validation in the Visitor
class. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
class IntegerRange: | ||
pass | ||
def __init__( | ||
self, | ||
min_amount: int, | ||
max_amount: int | ||
) -> None: | ||
self.min_amount = min_amount | ||
self.max_amount = max_amount | ||
|
||
def __set_name__(self, instance: "Visitor", name: str) -> None: | ||
self.protected_name = "_" + name | ||
|
||
def __get__(self, instance: "Visitor", owner: object) -> object: | ||
return getattr(instance, self.protected_name) | ||
|
||
def __set__(self, instance: "Visitor", value: int) -> None: | ||
if not isinstance(value, int): | ||
raise TypeError("Expected an integer value") | ||
if not (self.min_amount <= value <= self.max_amount): | ||
raise ValueError("Value out of the allowed range") | ||
setattr(instance, self.protected_name, value) |
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 IntegerRange
class is a descriptor that validates if a value is an integer and within a specific range. This is a good use of descriptors to enforce data validation.
class Visitor: | ||
pass | ||
|
||
def __init__(self, name: str, age: int, weight: int, height: int) -> None: | ||
self.name = name | ||
self.age = age | ||
self.weight = weight | ||
self.height = height |
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 Visitor
class is a simple data class that holds information about a visitor. It might be a good idea to add some data validation in the __init__
method to ensure that the name
is a string and that age
, weight
, and height
are integers.
class SlideLimitationValidator(ABC): | ||
pass | ||
def __init__(self, age: int, weight: int, height: int) -> None: | ||
self.age = age | ||
self.weight = weight | ||
self.height = height |
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 SlideLimitationValidator
class is an abstract base class that defines the interface for all slide limitation validators. This is a good use of the ABC module to define a common interface.
class ChildrenSlideLimitationValidator(SlideLimitationValidator): | ||
pass | ||
age = IntegerRange(4, 14) | ||
weight = IntegerRange(20, 50) | ||
height = IntegerRange(80, 120) | ||
|
||
|
||
class AdultSlideLimitationValidator(SlideLimitationValidator): | ||
pass | ||
age = IntegerRange(14, 60) | ||
weight = IntegerRange(50, 120) | ||
height = IntegerRange(120, 220) |
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 ChildrenSlideLimitationValidator
and AdultSlideLimitationValidator
classes inherit from SlideLimitationValidator
and define specific age, weight, and height limitations for children and adults. This is a good use of inheritance to create specific behavior based on the type of visitor.
class Slide: | ||
pass | ||
def __init__(self, name: str, | ||
limitation_class: SlideLimitationValidator) -> None: | ||
self.name = name | ||
self.limitation_class = limitation_class | ||
|
||
def can_access(self, visitor: Visitor) -> bool: | ||
try: | ||
self.limitation_class(visitor.age, visitor.weight, visitor.height) | ||
except (ValueError, TypeError)\ | ||
: | ||
return False | ||
else: | ||
return True |
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 Slide
class represents a slide in the park. It uses a SlideLimitationValidator
to determine if a visitor can access the slide. This is a good example of composition, where a class uses another class to achieve its functionality. However, the can_access
method is currently catching ValueError
and TypeError
exceptions and returning False
if they are raised. This is not a good practice, because it's hiding the real error. It would be better to let the exception propagate up and handle it at a higher level in the code.
No description provided.