Solution #1431
Solution #1431
Conversation
There was a problem hiding this comment.
Great job on this Pull Request! 🎉 Your code is generally well-written and follows good practices. However, there are some minor issues and potential improvements that you could consider for your future tasks. Keep up the good work and happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| def movie_session( | ||
| self, movie_name: str, | ||
| customers: list[Customer], | ||
| cleaning_staff: Cleaner | ||
| ) -> None: |
There was a problem hiding this comment.
In the movie_session method, you are passing customers as a list of Customer objects and cleaning_staff as a Cleaner object. This tightly couples your CinemaHall class to the Customer and Cleaner classes, making your code less flexible and harder to maintain. A better approach would be to pass these as generic objects and use duck typing. This way, you can pass any object that has a watch_movie method for customers and a clean_hall method for cleaning_staff. This is a code quality issue.
| print(f"\"{movie_name}\" started in hall number {self.number}.") | ||
| for customer in customers: | ||
| customer.watch_movie(movie_name) | ||
| print(f"\"{movie_name}\" ended.") |
There was a problem hiding this comment.
You are using print statements to log the start and end of the movie. While this is fine for a simple script, in a larger application you would want to use a logging framework. This would give you more control over your log messages, such as the ability to write them to a file or send them to a remote server, and to control their format and level of detail. This is a code quality recommendation.
| # write you code here | ||
| pass | ||
| def cinema_visit( | ||
| customers: list[dict], hall_number: int, cleaner: str, movie: str |
There was a problem hiding this comment.
The function cinema_visit is doing a lot of things, from creating customer objects to selling products and starting a movie session. Consider breaking down this function into smaller functions, each with a single responsibility. This would make the code easier to understand, test, and maintain.
| Customer( | ||
| name=customer["name"], food=customer["food"] | ||
| ) for customer in customers |
There was a problem hiding this comment.
Here you're creating Customer objects based on the input list of dictionaries. It might be better to handle this elsewhere, perhaps in a separate factory function or in the Customer class itself. This way, the cinema_visit function would not need to know the internal structure of the customer data, making it more flexible and easier to change in the future.
| cinema_bar = CinemaBar() | ||
| cleaner_ = Cleaner(name=cleaner) | ||
| for customer in customer_lst: | ||
| cinema_bar.sell_product(customer=customer, product=customer.food) |
There was a problem hiding this comment.
You are accessing customer.food directly here. It would be better to use a getter method to retrieve this information, to ensure that you're always getting the most up-to-date value and to encapsulate the internal data of the Customer class.
| for customer in customer_lst: | ||
| cinema_bar.sell_product(customer=customer, product=customer.food) | ||
| cinema_hall.movie_session( | ||
| movie_name=movie, customers=customer_lst, cleaning_staff=cleaner_ |
There was a problem hiding this comment.
The movie_session method seems to be doing a lot of things, including managing the customers and the cleaning staff. Consider breaking this down into smaller methods, each with a single responsibility. This would make the code easier to understand, test, and maintain.
| @@ -0,0 +1,6 @@ | |||
| class Cleaner: | |||
| def __init__(self, name: str) -> None: | |||
| self.name = name | |||
There was a problem hiding this comment.
As a good practice, it's recommended to use docstrings to provide a brief description about what the class or method does. This will enhance readability and understanding of your code.
| self.name = name | ||
|
|
||
| def clean_hall(self, hall_number: int) -> None: | ||
| print(f"Cleaner {self.name} is cleaning hall number {hall_number}.") |
There was a problem hiding this comment.
Avoid using print statements for non-debugging purposes. If you want to log some information, consider using Python's logging module. It provides a flexible framework for emitting log messages from Python programs.
| def __init__(self, name: str, food: str) -> None: | ||
| self.name = name | ||
| self.food = food |
There was a problem hiding this comment.
It might be a good idea to add some input validation for the name and food parameters. For instance, you could check if they are non-empty strings. This would prevent potential issues later on when these values are used.
No description provided.