Conversation
| private final Map<Integer, Film> films = new HashMap<>(); | ||
|
|
||
| @Autowired | ||
| private FilmService filmService; |
There was a problem hiding this comment.
Лучше инжектить зависимости через конструктор
| } | ||
|
|
||
| public Map<Film, Set<Integer>> addLike(int filmId, int userId) { | ||
| inMemoryUserStorage.validateUserId(userId); |
There was a problem hiding this comment.
Вызов метода validateUserId можно вынести в метод addLike
| } | ||
|
|
||
| public void removeLike(int filmId, int userId) { | ||
| inMemoryUserStorage.validateUserId(userId); |
|
|
||
| Collection<Film> getFilmsByLike(Integer sizeFilms); | ||
|
|
||
| void validateFilmId(Integer id); |
There was a problem hiding this comment.
Кажется 4 последних метода лучше сделать приватными
|
|
||
| @Override | ||
| public Collection<Film> getAll() { | ||
| log.debug("GET, all films"); |
There was a problem hiding this comment.
Уровень info будет достаточно для всех информационных логов
| String newEmail = newUser.getEmail(); | ||
| String newLogin = newUser.getLogin(); | ||
| oldUser.setName(newUser.getName()); | ||
| if (newUser.getBirthday() != null) oldUser.setBirthday(newUser.getBirthday()); |
There was a problem hiding this comment.
Лучше if указывать в фигурных скобках, так код становится более читаемым
| validateUserId(userId); | ||
| validateUserId(friendsId); | ||
|
|
||
| if (checkUserFriends(userId)) { |
There was a problem hiding this comment.
Блок можно переписать в одну строчку:
if (checkUserFriends(userId)) {
userFriends.get(userId).add(friendsId);
}
| } | ||
| } | ||
|
|
||
| if (checkUserFriends(friendsId)) { |
There was a problem hiding this comment.
Кажется лишняя проверка, если у первого есть коллекция друзей, то и у второго тоже они будут, так как процесс добавления синхронный
| Set<Integer> user2FriendsId = userFriends.get(friendsId); | ||
| List<User> mutualFriends = new ArrayList<>(); | ||
|
|
||
| for (int id1 : user1FriendsId) { |
There was a problem hiding this comment.
Попробуй вместо двойного цикла использовать метод intersect у коллекции set
|
|
||
| Collection<User> getMutualFriends(int userId, int friendsId); | ||
|
|
||
| void validateUserId(int id); |
There was a problem hiding this comment.
Последние 5 методов можно сделать приватными
|
|
||
| private final Map<Integer, Film> films = new HashMap<>(); | ||
| private final Map<Integer, Set<Integer>> filmsLikes = new HashMap<>(); | ||
| LocalDate birthdayFilm = LocalDate.of(1895, 12, 28); |
There was a problem hiding this comment.
Так как это констатнта, то следует указать private final
| } | ||
|
|
||
| @Override | ||
| public Collection<Film> getFilmsByLike(Integer sizeFilms) { |
There was a problem hiding this comment.
Смысл этого метода в том, чтобы вернуть определенный размер популярных фильмы по лайкам, он реализуется путем сортировки массива films по лайкам и получения размера массива в зависимости от параметра sizeFilms, то есть реализацию можно записать таким образом:
public Collection<Film> getFilmsByLike(Integer sizeFilms) {
return films.values()
.stream()
.sorted(Comparator.comparing(Film::getLikes).reversed())
.limit(sizeFilms)
.collect(Collectors.toList());
}
| Film film = films.get(filmId); | ||
|
|
||
| film.setLikes(film.getLikes() + 1); | ||
| films.put(filmId, film); |
There was a problem hiding this comment.
put в данном случае лишний, так как объект film выбирается по ссылке
|
|
||
| if (like != 0) { | ||
| film.setLikes(like - 1); | ||
| films.put(filmId, film); |
| if (friendsIds1.isEmpty()) { | ||
| userFriends.remove(userId); | ||
| } else { | ||
| userFriends.put(userId, friendsIds1); |
There was a problem hiding this comment.
put лишний, так как объект friendsIds1 выбирается по ссылке
| if (friendsIds2.isEmpty()) { | ||
| userFriends.remove(friendsId); | ||
| } else { | ||
| userFriends.put(friendsId, friendsIds2); |
2 participants
No description provided.