solution #616
solution #616
Conversation
There was a problem hiding this comment.
Great job on your code! 👍 There are just a few areas that need improvement. Please add docstrings to your classes and methods to describe their purpose and usage. Also, there seems to be a bug in your limitation class instantiation, please check it. Lastly, consider making your error messages more descriptive for better user understanding. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -2,24 +2,66 @@ | |||
|
|
|||
|
|
|||
| class IntegerRange: | |||
There was a problem hiding this comment.
It is a good practice to provide a docstring for the class to explain its purpose and how it should be used.
app/main.py
Outdated
| def __set__(self, instance: object, value: int) -> None: | ||
| if not self.min_amount <= value <= self.max_amount: | ||
| raise ValueError(f"must be in range:" # noqa | ||
| f" {self.min_amount} - {self.max_amount}") | ||
| setattr(instance, self.protected_name, value) |
There was a problem hiding this comment.
It's better to make error messages more descriptive. For example, instead of 'must be in range', you could specify what exactly should be in range.
| if not self.min_amount <= value <= self.max_amount: | ||
| raise ValueError(f"must be in range:" # noqa | ||
| f" {self.min_amount} - {self.max_amount}") | ||
| setattr(instance, self.protected_name, value) | ||
|
|
||
|
|
||
| class Visitor: |
There was a problem hiding this comment.
The 'Visitor' class could use a docstring to explain its purpose and usage.
| self.name = name | ||
| self.age = age | ||
| self.weight = weight | ||
| self.height = height | ||
|
|
||
|
|
||
| class SlideLimitationValidator(ABC): |
There was a problem hiding this comment.
The 'SlideLimitationValidator' class could use a docstring to explain its purpose and usage.
| height: int) -> None: | ||
| self.age = age | ||
| self.weight = weight | ||
| self.height = height | ||
|
|
||
|
|
||
| class ChildrenSlideLimitationValidator(SlideLimitationValidator): |
There was a problem hiding this comment.
The 'ChildrenSlideLimitationValidator' class could use a docstring to explain its purpose and usage.
| pass | ||
| age = IntegerRange(4, 14) | ||
| height = IntegerRange(80, 120) | ||
| weight = IntegerRange(20, 50) | ||
|
|
||
|
|
||
| class AdultSlideLimitationValidator(SlideLimitationValidator): |
There was a problem hiding this comment.
The 'AdultSlideLimitationValidator' class could use a docstring to explain its purpose and usage.
| pass | ||
| age = IntegerRange(14, 60) | ||
| height = IntegerRange(120, 220) | ||
| weight = IntegerRange(50, 120) | ||
|
|
||
|
|
||
| class Slide: |
There was a problem hiding this comment.
The 'Slide' class could use a docstring to explain its purpose and usage.
app/main.py
Outdated
| def can_access(self, visitor: Visitor) -> bool: | ||
| try: | ||
| self.limitation_class(visitor.age, visitor.weight, visitor.height) | ||
| return True | ||
| except ValueError: | ||
| return False |
There was a problem hiding this comment.
The 'can_access' method could use a docstring to explain its purpose and usage.
|
|
||
| def can_access(self, visitor: Visitor) -> bool: | ||
| try: | ||
| self.limitation_class(visitor.age, visitor.weight, visitor.height) |
There was a problem hiding this comment.
This line seems to be creating a new instance of a limitation class but does nothing with it. This is likely a bug, as it doesn't validate anything. You probably wanted to create an instance and then call a method on it to validate the visitor's parameters.
There was a problem hiding this comment.
Great job so far! 😊 However, there are some crucial improvements needed. Please make sure to handle possible exceptions in the can_access method more robustly and consider using the IntegerRange descriptor in the Visitor class to enforce attribute restrictions. Also, add a type check in the __set__ method of your descriptor. 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: object, value: int) -> None: | ||
| if not self.min_amount <= value <= self.max_amount: | ||
| raise ValueError(f"{value} must be in range:" # noqa | ||
| f" {self.min_amount} - {self.max_amount}") | ||
| setattr(instance, self.protected_name, value) |
There was a problem hiding this comment.
This is a potential code quality issue. In the __set__ method of your descriptor, you are not checking the type of the value being set. This could lead to unexpected behavior or errors if a non-integer value is used. Consider adding a check to ensure that the value is indeed an integer before proceeding with the range 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.
This is a design issue. The Visitor class has no restrictions on the values of age, weight, and height. However, in your SlideLimitationValidator subclasses, you are imposing restrictions on these attributes. This could lead to inconsistencies or errors. You might want to consider using your IntegerRange descriptor in the Visitor class to ensure that the values of age, weight, and height are always within acceptable ranges.
app/main.py
Outdated
| def can_access(self, visitor: Visitor) -> bool: | ||
| try: | ||
| self.limitation_class(visitor.age, visitor.weight, visitor.height) | ||
| except ValueError: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
This is a code quality issue. The can_access method in the Slide class is not very robust. It only catches ValueError, but other types of exceptions could potentially occur. For instance, if visitor is None or does not have the expected attributes (age, weight, height), an AttributeError would be raised. Consider improving your error handling to make your code more robust.
There was a problem hiding this comment.
Despite a few issues, your code is generally well written and clear. There are a few areas that need attention, such as the instantiation of limitation_class, the usage of IntegerRange descriptor in SlideLimitationValidator class, and the lack of abstract methods in the SlideLimitationValidator class. Consider these points as you continue to refine your code. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| self.name = name | ||
| self.age = age | ||
| self.weight = weight | ||
| self.height = height | ||
|
|
||
|
|
||
| class SlideLimitationValidator(ABC): |
There was a problem hiding this comment.
The SlideLimitationValidator class is declared as an abstract base class (ABC) but it doesn't have any abstract methods. If you intend to use it as an interface, you should define some abstract methods.
| def __init__(self, | ||
| age: int, | ||
| weight: int, | ||
| height: int) -> None: |
There was a problem hiding this comment.
The SlideLimitationValidator class and its subclasses are not using the IntegerRange descriptor for age, weight, and height attributes. You should use them in these classes.
|
|
||
| def can_access(self, visitor: Visitor) -> bool: | ||
| try: | ||
| self.limitation_class(visitor.age, visitor.weight, visitor.height) |
There was a problem hiding this comment.
The limitation_class is being instantiated with visitor's age, weight, and height. However, the SlideLimitationValidator class and its subclasses don't have a logic to check these values in the constructor. You should move the logic from IntegerRange's set method to init method or add a new method to check these values.
No description provided.