Conversation
Общий вывод по проектуПроект представляет собой классическое веб-приложение на базе сервлетов. Видна хорошая попытка структурировать код по слоям (Repository, Service, Servlet). Однако основные проблемы кроются в нарушении принципов работы с многопоточностью (использование HashMap в сервлетах) и ошибках в использовании обобщений (Generics) в интерфейсе репозитория. Также стоит уделить внимание обработке исключений и инкапсуляции полей. Итоговая оценка: C (deadline) |
| import java.time.LocalDateTime; | ||
|
|
||
| public class GameRecord { | ||
| private LocalDateTime timestamp; |
There was a problem hiding this comment.
Поля класса рекомендуется делать финальными, если объект не предполагает изменения после создания. Это обеспечивает потокобезопасность и упрощает логику.
| this.score = score; | ||
| } | ||
|
|
||
| public void setWin(boolean win) { |
There was a problem hiding this comment.
Имя сеттера 'setWin' не совсем соответствует имени поля 'isWin'. Рекомендуется использовать 'setWon' или переименовать поле в 'win' для единообразия с 'isWin()'.
|
|
||
| import java.io.IOException; | ||
| import java.util.Collection; | ||
|
|
There was a problem hiding this comment.
Прямая зависимость от синглтона 'UserService.USER_SERVICE' затрудняет тестирование. Рекомендуется использовать внедрение зависимостей (Dependency Injection).
| public class UserListServlet extends HttpServlet { | ||
|
|
||
| private final UserService userService= Winter.find(UserService.class); | ||
| private final UserService userService = UserService.USER_SERVICE; |
There was a problem hiding this comment.
Путь к JSP-файлу жестко закодирован в методе. Подобные константы следует выносить в отдельные статические переменные или конфигурацию.
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
|
|
||
|
|
There was a problem hiding this comment.
Использование пустой строки '""' в URL-паттерне может привести к конфликтам при обработке корневых запросов. Лучше использовать конкретный путь.
|
|
||
| public class UserRepository implements Repository<User> { | ||
|
|
||
| private final Map<Long, User> map = new HashMap<>(); |
There was a problem hiding this comment.
Использование 'HashMap' в многопоточной среде сервлетов небезопасно. Рекомендуется заменить на 'ConcurrentHashMap' для предотвращения проблем с консистентностью данных.
|
|
||
| private final Map<Long, User> map = new HashMap<>(); | ||
|
|
||
| public static final AtomicLong id = new AtomicLong(System.currentTimeMillis()); |
There was a problem hiding this comment.
Статическое поле для генерации ID в репозитории — плохая практика. Состояние счетчика должно принадлежать экземпляру или внешнему хранилищу, чтобы избежать проблем при тестировании.
|
|
||
| private final Map<Long, User> map = new HashMap<>(); | ||
|
|
||
| public static final AtomicLong id = new AtomicLong(System.currentTimeMillis()); |
There was a problem hiding this comment.
Использование 'System.currentTimeMillis()' в качестве начального значения 'AtomicLong' может привести к коллизиям ID при быстром перезапуске приложения.
| import com.javarush.babkin.entity.Role; | ||
| import com.javarush.babkin.entity.User; | ||
|
|
||
| import java.util.*; |
There was a problem hiding this comment.
Использование импорта через звездочку 'java.util.*' считается плохим тоном. Следует импортировать только те классы, которые действительно используются.
|
|
||
|
|
||
| public UserRepository() { | ||
| map.put(1L, User.builder() |
There was a problem hiding this comment.
Инициализация данных в конструкторе репозитория смешивает логику хранения и начального наполнения. Рекомендуется вынести наполнение тестовыми данными в отдельный сервис инициализации.
|
|
||
| @Override | ||
| public Collection<User> getAll() { | ||
| return map.values(); |
There was a problem hiding this comment.
Метод возвращает внутреннюю коллекцию напрямую. Рекомендуется возвращать неизменяемую копию (например, через 'List.copyOf()'), чтобы защитить данные от внешнего изменения.
| @@ -26,30 +26,42 @@ public void init(ServletConfig config) throws ServletException { | |||
|
|
|||
There was a problem hiding this comment.
Метод 'Long.parseLong(id)' может выбросить 'NumberFormatException', если параметр отсутствует или не является числом. Необходимо добавить проверку или блок try-catch.
| @@ -26,30 +26,42 @@ public void init(ServletConfig config) throws ServletException { | |||
|
|
|||
| @Override | |||
| protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { | |||
There was a problem hiding this comment.
Если пользователь с указанным ID не найден, сервлет продолжит выполнение без уведомления об ошибке. Следует предусмотреть обработку отсутствующего значения (например, редирект на 404).
|
|
||
| @WebServlet(value = "/edit-user", loadOnStartup = 1) | ||
| public class EditUserServlet extends HttpServlet { | ||
|
|
There was a problem hiding this comment.
Установка атрибута 'roles' в 'ServletContext' при каждом создании сервлета избыточна. Это лучше сделать один раз в 'ServletContextListener'.
|
|
||
| @Override | ||
| protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { | ||
| String stringId = req.getParameter("id"); |
There was a problem hiding this comment.
Закомментированный код ('// if (user.isPresent()) ...') следует удалять перед публикацией в репозиторий для поддержания чистоты проекта.
| import java.util.Collection; | ||
| import java.util.Optional; | ||
|
|
||
| public enum UserService { |
There was a problem hiding this comment.
Реализация сервиса в виде 'enum' (Singleton) ограничивает гибкость и затрудняет расширение приложения. Рекомендуется использовать обычный класс.
| public enum UserService { | ||
| USER_SERVICE; | ||
|
|
||
| public final Repository<User> userRepository = new UserRepository(); |
There was a problem hiding this comment.
Поле 'userRepository' объявлено как 'public'. Это нарушает принцип инкапсуляции. Поле должно быть 'private' и доступно только внутри сервиса.
| return userRepository.get(id); | ||
| } | ||
|
|
||
| public User getUserById(long userId) { |
There was a problem hiding this comment.
Методы 'getById' и 'getUserById' дублируют друг друга. Следует оставить один метод с четким возвращаемым типом.
| } | ||
|
|
||
| public User getUserById(long userId) { | ||
| return userRepository.get(userId).orElse(null); |
There was a problem hiding this comment.
Возврат 'null' через 'orElse(null)' в современном Java считается плохой практикой. Рекомендуется возвращать 'Optional' и обрабатывать его на уровне выше.
| return userRepository.get(userId).orElse(null); | ||
| } | ||
|
|
||
| public void updateQuestProgress(Long userId, QuestProgress progress) { |
There was a problem hiding this comment.
Метод 'updateQuestProgress' не содержит реализации в предоставленном коде или выглядит как проброс метода. Стоит добавить логирование или проверку входных данных.
| @Override | ||
| public void create(User entity) { | ||
| entity.setId(id.incrementAndGet()); | ||
| update(entity); |
There was a problem hiding this comment.
Метод 'create' вызывает 'update'. Хотя логика верна, для ясности кода лучше явно выполнять сохранение в карту внутри метода 'create'.
| @@ -1,27 +1,28 @@ | |||
| package com.javarush.khmelov.lesson14.controller; | |||
There was a problem hiding this comment.
В проекте полностью отсутствует логирование (например, через SLF4J). Использование 'System.out.println' или отсутствие логов затрудняет отладку в продакшене.
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
|
|
||
|
|
There was a problem hiding this comment.
Отсутствует валидация данных, поступающих из 'HttpServletRequest'. Это может привести к уязвимостям или ошибкам при обработке некорректного ввода.
| import java.util.*; | ||
| import java.util.concurrent.atomic.AtomicLong; | ||
|
|
||
| public class UserRepository implements Repository<User> { |
There was a problem hiding this comment.
Класс не помечен как 'final', хотя используется как единственный экземпляр. Рекомендуется явно ограничивать наследование, если оно не предполагается архитектурой.
No description provided.