-
Notifications
You must be signed in to change notification settings - Fork 859
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 #910
base: master
Are you sure you want to change the base?
solution #910
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.
You're doing a great job! 😊 However, there are a few points from the checklist that you missed. Please, consider them and make the necessary changes to your code. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
db/models.py
Outdated
from django.db import models | ||
from django.db.models import UniqueConstraint | ||
|
||
import settings |
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.
Code Style Issue: According to the checklist, you should use get_user_model()
instead of settings.AUTH_USER_MODEL
. It is considered best practice. Replace settings.AUTH_USER_MODEL
with get_user_model()
.
user = models.ForeignKey( | ||
settings.AUTH_USER_MODEL, on_delete=models.CASCADE |
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.
Code Style Issue: According to the checklist, you should define class Meta
after all attributes are defined. Please move class Meta
after the user
attribute.
services/order.py
Outdated
from django.db import transaction | ||
from django.db.models import QuerySet | ||
|
||
from db.models import Order, Ticket, User |
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.
According to the checklist, use get_user_model()
instead of User
. It is the best practice.
services/order.py
Outdated
order = Order.objects.create(user=User.objects.get(username=username)) | ||
if date: | ||
order.created_at = date | ||
order.save() |
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 are calling save()
method two times if the date
is not None
. According to the checklist, you should call save()
method only one time if it's possible. You can create Order
instance with created_at
field if date
is not None
.
tickets: list[dict], | ||
username: str, | ||
date: str = None | ||
) -> Order: |
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 tickets
parameter is a list of dictionaries. It's better to use a more descriptive name for it, for example, ticket_data_list
. It will make your code more readable.
services/order.py
Outdated
def create_order( | ||
tickets: list[dict], | ||
username: str, | ||
date: str = None | ||
) -> Order: | ||
with transaction.atomic(): | ||
order = Order.objects.create(user=User.objects.get(username=username)) | ||
if date: | ||
order.created_at = date | ||
order.save() | ||
for ticket in tickets: | ||
Ticket.objects.create( | ||
movie_session_id=ticket["movie_session"], | ||
order=order, | ||
row=ticket["row"], | ||
seat=ticket["seat"] | ||
) | ||
return order |
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. Your function is doing too much. It's creating an order, optionally setting a date, and creating tickets. Consider breaking it up into smaller functions, each with a single responsibility. This will make your code easier to read, test, and maintain.
@@ -0,0 +1,46 @@ | |||
from db.models import User |
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.
According to the checklist, it's better to use get_user_model()
instead of User
. It's a best practice in Django, because it allows us to swap out the user model with a custom one.
services/user.py
Outdated
user = User.objects.create_user(username=username, password=password) | ||
if email: | ||
user.email = email | ||
if first_name: | ||
user.first_name = first_name | ||
if last_name: | ||
user.last_name = last_name | ||
user.save() |
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're calling save()
method multiple times. It's not efficient and it's against our checklist. You should call save()
only once after all the changes. In your case, you can call save()
method right before returning the user object.
user = get_user(user_id) | ||
if username: | ||
user.username = username | ||
if password: | ||
user.set_password(password) | ||
if email: | ||
user.email = email | ||
if first_name: | ||
user.first_name = first_name | ||
if last_name: | ||
user.last_name = last_name | ||
user.save() |
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 same issue with save()
method here. Try to call save()
only once at the end of your function.
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 work so far! 😊 However, there are a few issues that need to be addressed before we can approve this Pull Request. Please review the comments about using get_user_model()
, calling save()
only once, and ensuring all attributes are defined before implementing class Meta
. Keep up the good work, you're almost there! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
db/models.py
Outdated
from django.db import models | ||
from django.db.models import UniqueConstraint | ||
|
||
import settings |
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 should use from django.conf import settings
instead of import settings
. The settings that you're importing here are not Django's settings. This could lead to unexpected behavior.
user = models.ForeignKey( | ||
settings.AUTH_USER_MODEL, on_delete=models.CASCADE |
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 line with the checklist, you should use get_user_model()
instead of settings.AUTH_USER_MODEL
. This is a best practice that avoids potential issues if the AUTH_USER_MODEL
changes during the lifecycle of the project.
|
||
class Order(models.Model): | ||
created_at = models.DateTimeField(auto_now_add=True) | ||
user = models.ForeignKey( |
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.
According to the checklist, you should ensure that all attributes are defined before implementing class Meta
. Please move the user
attribute definition above the class Meta
.
) | ||
if date: | ||
order.created_at = date | ||
order.save() |
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.
Don't Repeat Yourself: You are saving the order
object twice in your create_order
function. According to the checklist, you should call the save()
method only once if it's possible. Here, you can move the order.save()
call outside of the if
statement and save the order
after all modifications. This will improve your code performance.
order = Order.objects.create( | ||
user=get_user_model().objects.get(username=username) | ||
) |
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.
Code Style: According to the checklist, you should use one style of quotes in your code. You are using single quotes in some places and double quotes in other places. Please, use double quotes consistently throughout your code.
order = Order.objects.create( | ||
user=get_user_model().objects.get(username=username) | ||
) |
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.
Code Style: According to the checklist, you should use get_user_model()
instead of User
. You are doing this correctly here, which is a good practice.
order = Order.objects.create( | ||
user=get_user_model().objects.get(username=username) | ||
) |
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.
Clean Code: Your code is clean and easy to read. You have not left any unnecessary comments or print statements in your code. This is good practice according to the checklist.
@@ -0,0 +1,50 @@ | |||
from django.contrib.auth import get_user_model | |||
|
|||
from db.models import User |
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 are importing the User model directly from db.models, which is not a good practice. It's better to always use get_user_model() to get the user model. This function will always return the currently active user model – the custom user model if one is specified, or User otherwise.
user = get_user_model().objects.create_user( | ||
username=username, password=password | ||
) | ||
if email: | ||
user.email = email | ||
if first_name: | ||
user.first_name = first_name | ||
if last_name: | ||
user.last_name = last_name | ||
user.save() |
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 are calling the save() method multiple times. This is against the DRY principle. You should set all the attributes first, and then call save() only once.
user = get_user(user_id) | ||
if username: | ||
user.username = username | ||
if password: | ||
user.set_password(password) | ||
if email: | ||
user.email = email | ||
if first_name: | ||
user.first_name = first_name | ||
if last_name: | ||
user.last_name = last_name | ||
user.save() |
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.
Similar to the create_user method, you are calling the save() method multiple times. You should set all the attributes first, and then call save() only once.
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 work! There are a few suggestions and issues brought up in the review, including code style issues, unused imports, and some performance improvements. Please take some time to go through the comments and make the necessary adjustments. Keep up the good work and continue learning! 😊 Remember, the goal is not just to complete the task but also to learn and improve your skills.
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -28,13 +33,16 @@ def create_movie( | |||
genres_ids: list = None, | |||
actors_ids: list = None, | |||
) -> Movie: |
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.
According to the checklist, you should use double quotes in your code. Single quotes are used here.
if genres_ids: | ||
movie.genres.set(genres_ids) | ||
if actors_ids: | ||
movie.actors.set(actors_ids) | ||
|
||
return movie |
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 save()
method is called only once, which is good, but the code can be improved by moving the save()
method call to the end of the with transaction.atomic():
block. This way, if anything fails while setting the genres and actors, the movie won't be saved, which is a good practice when dealing with transactions.
@@ -42,3 +42,9 @@ def update_movie_session( | |||
|
|||
def delete_movie_session_by_id(session_id: int) -> None: | |||
MovieSession.objects.get(id=session_id).delete() |
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.
Code Quality: Similar to the previous comment, consider using get_object_or_404
function here.
) | ||
if date: | ||
order.created_at = date | ||
order.save() |
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.
Don't Repeat Yourself: You are calling the save()
method twice in this function. It would be more efficient to call it only once after all changes to the order
object have been made. Here is the algorithm you can use: 1. Create an order object without saving it to the database. 2. If the date
parameter is given, change the created_at
attribute of the order
object. 3. Save the order
object to the database. 4. Create the tickets.
order = Order.objects.create( | ||
user=get_user_model().objects.get(username=username) | ||
) |
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.
Code Style: It's better to use intermediate variables for complex expressions. This will make your code more readable. For example, you can create a variable for the user: user = get_user_model().objects.get(username=username)
and then use it in your code: Order.objects.create(user=user)
.
Ticket.objects.create( | ||
movie_session_id=ticket["movie_session"], | ||
order=order, | ||
row=ticket["row"], | ||
seat=ticket["seat"] | ||
) |
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.
Performance issue: You are creating a new Ticket
object for each ticket in the tickets
list. This will result in multiple database queries, which can be inefficient if the list is large. Consider using bulk_create
method to create all Ticket
objects in one database query. Here is the algorithm you can use: 1. Create a list of Ticket
objects without saving them to the database. 2. Use Ticket.objects.bulk_create(your_list)
to save all Ticket
objects to the database in one query.
@@ -0,0 +1,50 @@ | |||
from django.contrib.auth import get_user_model | |||
|
|||
from db.models import User |
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 import of the User model from db.models is not used in the code, so it should be removed. It's a code style issue. Also, remember from the checklist that it's best practice to use get_user_model()
instead of directly importing the User model.
email: str = None, | ||
first_name: str = None, | ||
last_name: str = None | ||
) -> User: |
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 function return type is User, but it should be the type returned by get_user_model()
. It's a code style issue.
return user | ||
|
||
|
||
def get_user(user_id: int) -> User: |
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 function return type is User, but it should be the type returned by get_user_model()
. It's a code style issue.
email: str = None, | ||
first_name: str = None, | ||
last_name: str = None | ||
) -> User: |
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 function return type is User, but it should be the type returned by get_user_model()
. It's a code style issue.
No description provided.