Conversation
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Привет, Вячеслав! Спасибо за PR.
Отличная работа! :)
Есть совсем небольшие пометки, посмотри комментарии ниже, пожалуйста
В первую очередь
- раз у нас есть для сервисов интерфейс+реализация, то инжектить стоит через интерфейс, именно этого будут все ожидать. Ну или убрать вовсе интерфейсы и инжектить реализации, но тогда названия пусть будут без Impl
- посмотри еще запросы в цикле - это потенциальные проблемы производительности :)
|
|
||
| private static final Logger log = LoggerFactory.getLogger(FilmController.class); | ||
| private final FilmService filmService; | ||
| private final FilmServiceImpl filmService; |
There was a problem hiding this comment.
FilmServiceImpl
Раз у нас есть интерфейс, то тут стоит именно его и инжектить, а не реализацию. В других местах аналогично :)
|
|
||
| for (Film film : films) { | ||
|
|
||
| getGenre(film); |
There was a problem hiding this comment.
Запрос в БД - дорогая операция, их лучше минимизировать, а тут же в цикле идут запросы для каждого элемента. Это не особо эффективно особенно если у нас будет много записей в БД.
По сути дела мы делаем запрос
select * from
можно заменить на
select * from
где ids - список идентификаторов :)
Так будет гораздо эффективней с точки зрения производительности
| for (Genre genre : genres) { | ||
| if (!duplicateGenres.contains(genre)) { | ||
| params.addValue("genre_id", genre.getId()); | ||
| jdbc.update("INSERT INTO FILM_GENRES (FILM_ID, GENRE_ID) VALUES(:film_id, :genre_id);", params, keyHolder); |
There was a problem hiding this comment.
Вот тут тоже цикле запросы, причем инсерты, что еще дороже чем селект в том же цикле. А что если склеить все запросы в одну строку
- как набор операторов через
;insert into t1() values (...); insert into t1() values (...); ... - как один оператор, примерно вот так insert into t1() values (...), (...),...(...);
|
|
||
| @Override | ||
| public Genre getGenreById(int id) throws DataNotFoundException { | ||
| if (id > 6) { |
There was a problem hiding this comment.
id > 6
А это уже "магия", а точно надо ограничивать идентификатор?
|
|
||
| @Override | ||
| public Mpa getById(int id) throws DataNotFoundException { | ||
| if (id > 5) { |
There was a problem hiding this comment.
id > 5
Магия, аналогично. Это выглядит как какой то неочевидный хак, никто потом не разберется почему так, лучше такого избегать, это неочевидно и совершенно неожиданно :)
| public class FilmControllerTest { | ||
|
|
||
| @Autowired | ||
| /* @Autowired |
There was a problem hiding this comment.
Лучше, конечно, тест не комментить а попробовать починить. Ну и сразу - контроллер инжектить не надо, надо запрос отправить :)
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Давай не будем, все таки, из таблицы БД все что есть тащить, отметил комментарием, посмотри. пожалуйста
| @Override | ||
| public Genre getGenreById(int id) throws DataNotFoundException { | ||
| if (id > 6) { | ||
| if (id > jdbcGenreRepository.getAll().size()) { |
There was a problem hiding this comment.
jdbcGenreRepository.getAll().size()
Это сурово, мы вытаскиваем все записи из таблицы чтобы проверить id, а если записей вдруг станет много, у нас кончится память, в общем, там лучше не делать
- тут поможет exists или count запрос https://stackoverflow.com/questions/7471625/fastest-check-if-row-exists-in-postgresql
- ну или самый простой вариант - ниже мы делаем запрос в БД, а что он вернет если нужной записи в БД не будет, может на это завязаться и все проще станет? :)
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Теперь все отлично, спасибо :)
3 participants
No description provided.