-
Notifications
You must be signed in to change notification settings - Fork 607
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 #615
base: master
Are you sure you want to change the base?
solution #615
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.
Your code is mostly good, but there are a few critical issues that need to be addressed. These include returning incorrect data types and not validating input properly. Once these issues are fixed, your code will be much more robust and reliable. Keep going! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
app/main.py
Outdated
instance: SlideLimitationValidator, | ||
value: int | ||
) -> None: | ||
if self.min_amount <= value <= self.max_amount: |
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 quality issue. You should check for invalid input and throw an exception if the input is invalid. Currently, if the value is not within the specified range, the attribute is not set at all, which could lead to confusing behavior.
app/main.py
Outdated
)) | ||
if len(result.__dict__) == 3: | ||
return True | ||
return 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.
This is a bug. Your can_access
method is supposed to return a Visitor object or None, but it actually returns a boolean. You should return the visitor if the conditions are met, or None otherwise.
app/main.py
Outdated
visitor.weight, | ||
visitor.height | ||
)) | ||
if len(result.__dict__) == 3: |
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 quality issue. Checking the length of the __dict__
attribute to determine if the object is valid is not a good practice. It might be better to define a method in the SlideLimitationValidator
class to check if the object is valid, and call that method here.
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 issues that need to be addressed. Please take a look at the type hinting in your code, as well as the class inheritance and data validation. Keep up the good work! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
def __set_name__( | ||
self, | ||
owner: SlideLimitationValidator, | ||
name: str | ||
) -> None: |
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 have a type hinting issue here. The owner
argument in the __set_name__
method is expected to be of SlideLimitationValidator
type, but you are passing self
which is of IntegerRange
type. Consider reviewing your type hints.
app/main.py
Outdated
def __get__( | ||
self, | ||
instance: SlideLimitationValidator, | ||
owner: SlideLimitationValidator | ||
) -> int: | ||
return getattr(instance, self.protected_name) |
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 issue here with the instance
argument. It's type hinted as SlideLimitationValidator
but self
of IntegerRange
type is being passed.
app/main.py
Outdated
def __set__( | ||
self, | ||
instance: SlideLimitationValidator, | ||
value: int | ||
) -> None: | ||
if not self.min_amount <= value <= self.max_amount: | ||
setattr(instance, self.protected_name, False) | ||
return |
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.
In the __set__
method, you are setting the attribute to False
if the value does not meet the condition. This might be confusing because the attribute can be an integer or a boolean. It would be more appropriate to raise an exception.
app/main.py
Outdated
class SlideLimitationValidator(ABC): | ||
pass | ||
def __init__( | ||
self, | ||
age: int, | ||
weight: int, | ||
height: int | ||
) -> None: | ||
self.age = age | ||
self.weight = weight | ||
self.height = height | ||
|
||
def checking(self) -> bool: | ||
if False in self.__dict__.values(): | ||
return False | ||
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 SlideLimitationValidator
class is inherited from ABC
which stands for Abstract Base Class, but you are not defining any abstract methods in it. If there are no abstract methods to enforce, you might not need to inherit from ABC
.
app/main.py
Outdated
def can_access(self, visitor: Visitor) -> Visitor | None: | ||
return self.limitation_class( | ||
visitor.age, | ||
visitor.weight, | ||
visitor.height | ||
).checking() |
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.
In the can_access
method, the return type is hinted as Visitor | None
, but you are returning the result of the checking
method which is a boolean. Consider revising your type hints or the return statement.
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 your code! It's clear and adheres to the SOLID principles. However, there are a few areas for improvement. In the can_access
method, consider passing the Visitor
object to the SlideLimitationValidator
for efficiency and scalability. The checking
method could be renamed to something more descriptive. Also, avoid violating the Single Responsibility Principle in the IntegerRange
class and refrain from using exceptions for flow control. Keep up the good work and keep these points in mind for your future coding tasks. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
) -> None: | ||
try: | ||
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.
It seems like you are raising a ValueError just to catch it immediately and change the result
to False. It's not a good practice to raise exceptions for flow control in Python. Instead, you could just use an if-else statement to set result
to False if the value is not within the range.
if not self.min_amount <= value <= self.max_amount: | ||
raise ValueError | ||
except ValueError: | ||
instance.result = 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.
The result
attribute is being set in the IntegerRange
class, which seems to be a violation of the Single Responsibility Principle. It would be better to have this logic in the SlideLimitationValidator
class, as it's related to the validation of the slide limitations, not to the operation of setting and getting values.
self.weight = weight | ||
self.height = height | ||
|
||
def checking(self) -> bool: |
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 checking
method is a bit unclear. The name of the method should be a verb or a verb phrase and should indicate what the method does. Consider renaming this method to something more descriptive, like is_valid
or check_validity
.
self.name = name | ||
self.limitation_class = limitation_class | ||
|
||
def can_access(self, visitor: Visitor) -> bool: |
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.
In the can_access
method, it would be more efficient to pass the Visitor
object to the SlideLimitationValidator
instead of passing each attribute individually. This would make the code more scalable, as you wouldn't need to change the SlideLimitationValidator
class and its subclasses if you were to add more attributes to the Visitor
class in the future.
No description provided.