Solution #1439
Solution #1439
Conversation
There was a problem hiding this comment.
Great work on your code! 😃 There are just a few areas that need a bit of polishing. Remember to add docstrings to your classes and methods for better readability and maintainability. Consider using more descriptive method names and efficient list comprehensions. Also, try to adhere to the Single Responsibility Principle and Dependency Inversion Principle for better code structure. 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! ✨
| class CinemaHall: | ||
| def __init__( | ||
| self, | ||
| number: int | ||
| ) -> None: |
There was a problem hiding this comment.
This class named CinemaHall has only one attribute number. It is a good practice to have more attributes that define the state of the object. For example, you can add attributes like seats, screen_size, sound_system, etc. to make it more representative of a real cinema hall.
| def movie_session( | ||
| self, | ||
| movie_name: str, | ||
| customers: list[Customer], | ||
| cleaning_staff: Cleaner | ||
| ) -> None: | ||
| print(f'"{movie_name}" started in hall number {self.number}.') | ||
| for customer in customers: | ||
| customer.watch_movie(movie_name) | ||
| print(f'"{movie_name}" ended.') | ||
| cleaning_staff.clean_hall(self.number) |
There was a problem hiding this comment.
The movie_session method seems to be doing too much. It is not only responsible for starting and ending the movie session, but also for making each customer watch the movie and making the cleaning staff clean the hall. This violates the Single Responsibility Principle. Consider splitting this method into several methods, each with a single responsibility. For example, you can create a start_session method, a end_session method, a watch_movie method in the Customer class, and a clean_hall method in the Cleaner class.
| self, | ||
| movie_name: str, | ||
| customers: list[Customer], | ||
| cleaning_staff: Cleaner | ||
| ) -> None: |
There was a problem hiding this comment.
The movie_session method accepts a list of Customer objects and a Cleaner object as parameters. This means that the CinemaHall class is tightly coupled with the Customer and Cleaner classes, which is not a good practice. Consider using the Dependency Inversion Principle to make your code more flexible and maintainable. This principle states that high-level modules should not depend on low-level modules. Both should depend on abstractions. In this case, you can create an abstract Person class and make both Customer and Cleaner inherit from it. Then you can make the CinemaHall class depend on the Person class instead of Customer and Cleaner.
| def cinema_visit( | ||
| customers: list, | ||
| hall_number: int, | ||
| cleaner: str, | ||
| movie: str |
There was a problem hiding this comment.
The function cinema_visit lacks a docstring. It's a good practice to include docstrings in your functions to describe their behavior, arguments, return values, and any exceptions they raise. This makes your code easier to understand and maintain.
app/main.py
Outdated
| visitors = [] | ||
| for customer in customers: | ||
| visitors.append(Customer(customer["name"], customer["food"])) |
There was a problem hiding this comment.
You're creating Customer objects within a loop and appending them to the visitors list. This is not efficient and can be improved. Consider using a list comprehension to create these objects in a single line of code. This would make your code cleaner and more Pythonic.
app/main.py
Outdated
| cinema_bar = CinemaBar() | ||
| cleaning_staff = Cleaner(cleaner) | ||
| for visitor in visitors: | ||
| cinema_bar.sell_product(visitor.food, visitor) |
There was a problem hiding this comment.
Here you are calling the sell_product method of cinema_bar object. It's not clear from the method name what it does. Consider renaming it to something more descriptive, like sell_food_to_visitor, to improve the readability of your code.
| cleaning_staff = Cleaner(cleaner) | ||
| for visitor in visitors: | ||
| cinema_bar.sell_product(visitor.food, visitor) | ||
| cinema_hall.movie_session(movie, visitors, cleaning_staff) |
There was a problem hiding this comment.
The movie_session method is being called here, but it's not clear what it does just by its name. Consider renaming it to something more descriptive, like start_movie_session, to improve the readability of your code.
app/main.py
Outdated
| cinema_bar = CinemaBar() | ||
| cleaning_staff = Cleaner(cleaner) | ||
| for visitor in visitors: | ||
| cinema_bar.sell_product(visitor.food, visitor) |
There was a problem hiding this comment.
No need to have instance of CinemaBar. You can use class CinemaBar directly as sell_product method is static method
app/cinema/bar.py
Outdated
| @@ -0,0 +1,8 @@ | |||
| from __future__ import annotations | |||
No description provided.