Conversation
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Привет, Вячеслав! Спасибо за PR.
В общем виде все верно, но хочется проработать некоторые моменты, избавится от некоторых двусмысленностей и сделать более единообразно, отметил комментариями, посмотри, пожалуйста :)
|
|
||
| @DeleteMapping(path = "/films/{id}/like/{userId}") | ||
| public void deleteLike(@PathVariable("id") int id, @PathVariable("userId") int userId) { | ||
| log.info("delete like: {}", id); |
There was a problem hiding this comment.
Давай контроллеры все таки логировать не будем, пусть все логи будут в сервисе. Тут у нас, по сути дела, чисто маршрутизация http-запросов на севисы, тут нет бизнес-логики. Если уж тут что то логировать, то http-запросы, но это лучше делать дополнительной библиотекой
https://errorsfixing.com/how-to-instantiate-logbook-in-spring-boot-app/
| @@ -0,0 +1,7 @@ | |||
| package ru.yandex.practicum.filmorate.exception; | |||
|
|
|||
| public class DataNotFoundException extends NullPointerException { | |||
There was a problem hiding this comment.
extends NullPointerException
Это совершенно разные ситуации, NPE - это ошибка в логике, передали null, чисто техническая история. А вот когда данные по идентификатору не найдены - это уже относится к бизнес-логике, тут лучше вообще проверяемое исключение кидать (наследника Exception) :)
| public class FilmService { | ||
|
|
||
| private final FilmStorage filmStorage; | ||
| private final UserStorage userStorage; |
There was a problem hiding this comment.
filmStorage ок
userStorage - а вот тут лучше заменить на сервис, userService, иначе мы двумя разными способами будем доставать сущности
| User user = userStorage.get(id); | ||
| User friend = userStorage.get(friendId); | ||
| if (user == null) { | ||
| throw new DataNotFoundException("Искомый пользователь не найден"); |
There was a problem hiding this comment.
Можно сделать метод User get(id), а вот в нем уже делать проверку и кидать исключение. Ну и доставать пользователя всегда этим методом - тогда у нас будет единообразие в логике :)
| log.debug("Фильм успешно изменён"); | ||
| return newFilm; | ||
| } | ||
| throw new DataNotFoundException("Некорректный Id"); |
There was a problem hiding this comment.
Имя переменной - данные не найдены, но сообщение о некорректном ид, как то не особо логично :)
|
|
||
| @Override | ||
| public Film get(int id) { | ||
| return films.get(id); |
There was a problem hiding this comment.
Во, здесь и делать проверку и кидать исключение, для пользователей в сервисе так же, тогда у нас будет везде единообразно :)
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Теперь все отлично, спасибо
Сколько лишнего дублирующегося кода ушло и насколько стало все лаконичней :)
3 participants
No description provided.