Skip to content
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 #873

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Solution #873

wants to merge 6 commits into from

Conversation

Ivan-Shakhman
Copy link

first try.. the game begining

Copy link

@MNDonut MNDonut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Squid Game Season 2 started 🔪

db/models.py Outdated

class Order(models.Model):
created_at = models.DateTimeField(auto_now_add=True)
user = models.ForeignKey(to=User, on_delete=models.CASCADE)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use AUTH_USER_MODEL instead because when use change the model inside this constant, then it won't be updated here

db/models.py Outdated
Comment on lines 77 to 82
related_name="ticket"
)
order = models.ForeignKey(
to=Order,
on_delete=models.CASCADE,
related_name="ticket"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure it's an appropriate name? related manager returns 1 object for FK or queryset?

db/models.py Outdated
Comment on lines 94 to 95
fields=["movie_session", "row", "seat"],
name="unique_ticket"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specify what fields are unique in index name so that other devs know it

Comment on lines 48 to 57
movie_session = MovieSession.objects.get(pk=movie_session_id)
result = []
for row in range(1, movie_session.cinema_hall.rows + 1):
for seat in range(1, movie_session.cinema_hall.seats_in_row + 1):
if Ticket.objects.filter(
movie_session=movie_session,
row=row,
seat=seat
).exists():
result.append({"row": row, "seat": seat})
Copy link

@MNDonut MNDonut Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow. An advice for you. Try to avoid such constructions that make 1 sql query on each iteration(when possible)
From performance perspective it's really bad idea because if your hall has 10 rows and 10 seats, it's 100 iterations meaning 100 queries in db and it's will take too much time to just fetch available seats.
The best way is to do everything using ORM that will fetch it in 1-2 sql queries. If you work with something bigger than cinema hall

(наприклад система продажу квартир у житловому комплексі, який має 6 корпусів, у кожному по 4 під'їзди, кожен комплекс має 15 поверхів і кожен поверх має 20 квартир, то в цьому випадку ти б зробив 4 цикли(кожен корпус, під'їзд, поверх і квартира) і це було б 6 * 4 * 15 * 20 = 7200 запитів до бази даних, щоб подивитися які квартири вільні й де саме. Якщо записів у бд небагато, то кожен запит це 1-2 секунди. Тобто ти б витратив 7200 або 14400 секунд, або ж 2-4 години, щоб просто у додатку відобразити всі квартири. Зазвичай люди 5 секунд не можуть почекати, а тут години)

Tip: you can use .values() method to get row and seat as a dict. Everything else you should find out by yourself ;)

Comment on lines 3 to 4
from db.models import Order, User, MovieSession
from db.models import Ticket
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you split this import?

Comment on lines 20 to 27
movie_session = MovieSession.objects.get(
pk=ticket["movie_session"]
)
Ticket.objects.create(
movie_session=movie_session,
order=order,
row=ticket["row"],
seat=ticket["seat"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to set movie_session_id to avoid redundant sql query

if username:
orders = orders.filter(user__username=username)

return orders.order_by("-created_at")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant ordering taking into accout that you added ordering in Order.Meta

services/user.py Outdated
users_info["first_name"] = first_name
if last_name:
users_info["last_name"] = last_name
created_user = User.objects.create_user(**users_info)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use User model, use get_user_model() instead. Fix everywhere

services/user.py Outdated
first_name: str | None = None,
last_name: str | None = None,
) -> User:
updated_user = User.objects.get(pk=user_id)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reuse get_user to follow DRY?

settings.py Outdated
Comment on lines 27 to 29
"db",
"django.contrib.auth",
"django.contrib.contenttypes",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the ordering must be:
default apps
your apps
third-party apps

.flake8 Outdated
@@ -6,5 +6,5 @@ max-complexity = 18
select = B,C,E,F,W,T4,B9,ANN,Q0,N8,VNE
exclude =
**migrations
venv
.venv

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert these changes on remote

db/models.py Outdated
from django.db import models
from django.db.models import UniqueConstraint

from settings import AUTH_USER_MODEL

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import from django,conf to ensure use of global proejct settings

db/models.py Outdated

class Order(models.Model):
created_at = models.DateTimeField(auto_now_add=True)
user = models.ForeignKey(to=AUTH_USER_MODEL, on_delete=models.CASCADE)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related name missing here

db/models.py Outdated
seat = models.IntegerField()

def __str__(self) -> str:
return f"{self.movie_session.movie.title} \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please adhere to the project coding style, \ is unacceptable for breaking lines here 🧐

return f"{self.movie_session.movie.title} \
{self.movie_session.show_time} (row: {self.row}, seat: {self.seat})"

class Meta:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a convention in Django Meta class is defined right after fields definition

db/models.py Outdated

if not (1 <= self.seat <= self.movie_session.cinema_hall.seats_in_row):
errors["seat"] = (
f"seat number must be in available range: \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try to break lines without this symbol really

Comment on lines 48 to 58
movie_session = MovieSession.objects.get(pk=movie_session_id)
tickets = Ticket.objects.filter(movie_session=movie_session)
result = []
for row in range(1, movie_session.cinema_hall.rows + 1):
for seat in range(1, movie_session.cinema_hall.seats_in_row + 1):
if tickets.filter(
row=row,
seat=seat
).exists():
result.append({"row": row, "seat": seat})
return result

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do it as a one liner with .values() method wrapped in list()

pk=ticket["movie_session"]
)
Ticket.objects.create(
movie_session=movie_session,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can pass movie_session_id to the manager instead of passing an object

manage.py Outdated
@@ -4,7 +4,7 @@


if __name__ == "__main__":
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "settings")
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "settings_dev")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the purpose of this change? did you change settings file?

Comment on lines 48 to 52
movie_session = MovieSession.objects.get(pk=movie_session_id)
tickets = Ticket.objects.filter(movie_session=movie_session)
result = []
for row in range(1, movie_session.cinema_hall.rows + 1):
for seat in range(1, movie_session.cinema_hall.seats_in_row + 1):
if tickets.filter(
row=row,
seat=seat
).exists():
result.append({"row": row, "seat": seat})
return result
result = (Ticket.objects.filter(movie_session=movie_session)
.values("row", "seat"))

return list(result)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do it using 1 orm query using movie session object + related name of tickets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants