Solution #683
Solution #683
Conversation
Could you please provide more details on what I need to refactor? I have seen another approved solution for this task and identified two areas for improvement:
|
cinema/models.py
Outdated
| def __str__(self) -> str: | ||
| return f"{self.name}: rows - {self.rows}, seats - {self.seats_in_row}" | ||
|
|
||
|
|
||
| class Movie(models.Model): | ||
| title = models.CharField(max_length=255) | ||
| description = models.TextField() | ||
| duration = models.IntegerField() |
There was a problem hiding this comment.
Using IntegerField for movie duration might work, but it's unclear what unit of time you're storing (minutes, seconds, etc.). It might be beneficial to add a help_text attribute or use DurationField for clarity and flexibility.
cinema/serializers.py
Outdated
| @@ -1,24 +1,63 @@ | |||
| from rest_framework import serializers | |||
|
|
|||
| from cinema.models import Movie | |||
| from cinema.models import Movie, Actor, Genre, CinemaHall | |||
There was a problem hiding this comment.
If importing more than two items, it is better to write them on separate lines. This improves readability and maintenance of the code.
Example:
From taxi.models import (
Driver,
Manufacturer,
Car
)
Read more about topic here
| ActorDetail, | ||
| CinemaHallViewSet, | ||
| ) | ||
|
|
cinema/serializers.py
Outdated
| @@ -1,24 +1,63 @@ | |||
| from rest_framework import serializers | |||
|
|
|||
| from cinema.models import Movie | |||
| from cinema.models import Movie, Actor, Genre, CinemaHall | |||
|
|
|||
|
|
|||
| class MovieSerializer(serializers.Serializer): | |||
There was a problem hiding this comment.
The MovieSerializer could be simplified by using ModelSerializer instead of Serializer. Since you're working with a Django model, ModelSerializer automatically handles common functionality (like field definitions and create/update methods) for you.
cinema/serializers.py
Outdated
| actors = validated_data.get("actors", None) | ||
| genres = validated_data.get("genres", None) | ||
|
|
There was a problem hiding this comment.
In the update method of MovieSerializer, the checks for actors and genres could be simplified. Instead of checking whether they exist and updating only if provided, you might directly set these values, as .set() works with empty lists as well.
cinema/views.py
Outdated
| class ActorList( | ||
| mixins.ListModelMixin, | ||
| mixins.CreateModelMixin, | ||
| generics.GenericAPIView | ||
| ): | ||
| queryset = Actor.objects.all() | ||
| serializer_class = ActorSerializer | ||
|
|
||
| def get(self, request: HttpRequest, *args, **kwargs) -> Response: | ||
| return self.list(request, *args, **kwargs) | ||
|
|
||
| def post(self, request: HttpRequest, *args, **kwargs) -> Response: | ||
| return self.create(request, *args, **kwargs) |
There was a problem hiding this comment.
In your ActorList and ActorDetail classes, the get and post methods could be simplified further by calling the methods directly.
There was a problem hiding this comment.
I cannot delete these methods because it will cause test failures in both classes
There was a problem hiding this comment.
OK. I've found the solution for this already. I have changed inherit, but in READMY.md write that for the Actor model I need to use a GenericAPIView
cinema/views.py
Outdated
| class ActorDetail( | ||
| mixins.RetrieveModelMixin, | ||
| mixins.UpdateModelMixin, | ||
| mixins.DestroyModelMixin, | ||
| generics.GenericAPIView, | ||
| ): | ||
| queryset = Actor.objects.all() | ||
| serializer_class = ActorSerializer | ||
|
|
||
| def get(self, request: HttpRequest, *args, **kwargs) -> Response: | ||
| return self.retrieve(request, *args, **kwargs) | ||
|
|
||
| def put(self, request: HttpRequest, *args, **kwargs) -> Response: | ||
| return self.update(request, *args, **kwargs) | ||
|
|
||
| def patch(self, request: HttpRequest, *args, **kwargs) -> Response: | ||
| return self.partial_update(request, *args, **kwargs) | ||
|
|
||
| def delete(self, request: HttpRequest, *args, **kwargs) -> Response: | ||
| return self.destroy(request, *args, **kwargs) |
There was a problem hiding this comment.
No need for overrides:
Since ListModelMixin provides the logic for listing and creating objects, and RetrieveModelMixin, UpdateModelMixin, and DestroyModelMixin handle retrieval, updating, and deletion, you don't need to redefine those methods.
No description provided.