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.

Sign up for GitHub

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

Jump to bottom

Add database #3

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add database #3

wants to merge 3 commits into from

Conversation

fiv1994
Copy link
Owner

@fiv1994 fiv1994 commented Jan 4, 2025

No description provided.

@Sla-als
Copy link

Sla-als commented Jan 5, 2025

Не нужно было делать новый пр и удалять старый

Sla-als
Sla-als reviewed Jan 5, 2025
View reviewed changes
Copy link

@Sla-als Sla-als left a comment

Choose a reason for hiding this comment

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

есть замечания

src/test/java/ru/yandex/practicum/filmorate/model/FilmTest.java Outdated
Copy link

Choose a reason for hiding this comment

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

В ТЗ требуется интеграционное тестирование с аннотациями @jdbcTest, @AutoConfigureTestDatabase, @import(...) (пример приведён в самом ТЗ). Сейчас в проекте тесты на валидацию моделей (FilmTest, UserTest), но не видно тестов, которые проверяют взаимодействие приложения с базой.

Copy link
Owner Author

Choose a reason for hiding this comment

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

src/test/java/ru/yandex/practicum/filmorate/FilmorateApplicationTests.java - здесь прописаны тесты на взаимодействие приложения с базой...

src/main/java/ru/yandex/practicum/filmorate/storage/film/FilmDbStorage.java Outdated
Copy link

Choose a reason for hiding this comment

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

🔥🔥🔥 Логика получения и маппинга жанров, а также MPA при запросах к БД реализована аккуратно и в одном запросе (через JOIN), нет лишних циклов, и данные подтягиваются одной выборкой.

src/main/java/ru/yandex/practicum/filmorate/model/Film.java Outdated

private RatingMPA mpa;

private Integer likes;
Copy link

Choose a reason for hiding this comment

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

в Film есть поле private Integer likes;. Похоже, оно используется для подсчёта общего количества лайков. Учитывая, что сама таблица лайков есть, и количество лайков уже собирается SQL-запросами, можно либо убрать это поле из модели, либо хранить там только «предвычисленное» значение и аккуратно поддерживать синхронизацию. Чтобы не путать логику (где-то хранится в БД, а где-то ещё и поле в объекте), обычно подсчёт лайков делают «на лету» через COUNT, как у вас и сделано.

@Sla-als
Copy link

Sla-als commented Jan 5, 2025

Все делайте в текущем ПР

@Sla-als
Copy link

Sla-als commented Jan 7, 2025

Супер

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sla-als Sla-als Sla-als left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fiv1994 @Sla-als