Conversation
…st. Changed it to @PastOrPresent in User
…rces (except test profile)
…tions with friends.
… with storage. Main Service for User.
… with storage. Main Service for Film.
…ler should not have. All operations go through UserService. Add new endpoints.
…ler should not have. All operations go through FilmService. Add new endpoints.
…ts and wrote new test for new endpoints.
…ge and TestEntity.
|
|
||
| /** | ||
| * Private constructor to initialize ID generator with starting ID = 1. | ||
| * Constructor for dependency injection |
There was a problem hiding this comment.
Вместо многострочного комментария лучше использовать Java Docs
| */ | ||
| @PutMapping | ||
| public User updateUser(@Valid @RequestBody User newUser) { | ||
| if (newUser.getId() == null) { |
There was a problem hiding this comment.
Этой проверке тоже н место в слое контроллера, всё убираем в сервисный слой.
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| public class Film { | ||
| public class Film extends StorageData { |
There was a problem hiding this comment.
Не нужное усложнение с наследованием от StorageData
Тут не прослеживается семейства, между фильмом и пользователем который его смотрит, то-есть наследование излишне.
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| public abstract class StorageData { | ||
| protected Long id; |
There was a problem hiding this comment.
Как я и сказал выше, излишний родитель ещё и только с идентификатором.
| * @see UserStorage | ||
| */ | ||
| @Service | ||
| public class FilmLikeService { |
There was a problem hiding this comment.
Сервисный слой лучше сделать через интерфейсы используя всю силу полиморфизма.
|
|
||
| public interface BasicStorage<T extends StorageData> { | ||
|
|
||
| T add(T t); |
There was a problem hiding this comment.
в результате тут свели преимущество java в жесткой типизации к минимуму, теперь имеем определение типа на уровне вызова методов, что крайне не удобно.
|
|
||
| @Override | ||
| public User add(User user) { | ||
| User returnUser = super.add(user); |
There was a problem hiding this comment.
метод выполняет разную логику а заявлено что это репозиторий, то-есть тут должно быть только добавление и ничего больше, вся остальная логика должна находится в сервисном слое.
|
|
||
| @Override | ||
| public User remove(Long id) { | ||
| throwIfNotFound(id); |
There was a problem hiding this comment.
Тут аналогично, и проверяем наличие, и кидаем ошибку, и создаём коллекции наполняя её, и логика работы в цикле со своей логикой лямбда выражения, в общем опять слишком много действий для класса репозитория и метода удаления, он должен просто удалить, всё остальное не должно быть в этом слое, это задача сервисного слоя.
|
|
||
| @Override | ||
| public void clear() { | ||
| super.clear(); |
| } | ||
|
|
||
| @Test | ||
| public void shouldThrowNotFoundWhenUserIsNotFoundReturnFriends() { |
There was a problem hiding this comment.
в каких то тестах есть модификаторы доступов у методов в каких то нет, это плохо, нужно привести к единообразию. Либо везде пишем либо везде не пишем, лучше писать.
2 participants
Вопросы/Проблемы
Буду очень рад услышать предложения по улучшению для этих поинтов(да и для других, если они есть)!
Всякое
Хранилище:
Основная идея - сохранить классы User и Film как можно более простыми, поэтому они получили только наследование от абстрактного класса StorageData. Film получил поле Long likes.
Хранилище реализовано при помощи интерфейсов и абстрактного класса (на схеме)
Map<userId, Map<friendId, User>>.Map<filmId, Set<userId>>.Немного требовательно, зато удобно.
Идея, где при каждом возврате пользователя/фильма мне одновременно приходит список друзей/пользователей который поставили лайки кажется мне странной.
Сервисы
За операции с друзьями отвечает FriendShipService - он вызывает методы хранилища и имеет свою логику.
За операции с пользователями отвечает UserService - он вызывает метода хранилища и методы FriendShipService - по большой части просто класс-обертка.
За операции с лайками отвечает FilmLikeService- он вызывает методы хранилища и имеет свои проверки(например проверку существования пользователя).
За операции с пользователями отвечает FilmService - он вызывает метода хранилища и методы FilmLikeService - по большой части просто класс-обертка.
Контроллеры
Знают только о своих сервисах. выполняют проверки через аннотации.
Тесты
Постарался улучшить качество и количество тестов при помощи mockito, для сервисов почти нет тестов, так как тестировать особо нечего
По неведомой мне причине в прошлый раз я начал писать javadoc на английском, так что пришлось поддерживать эту тенденцию.