mushanov | projectPantera#19
Conversation
demologin
left a comment
demologin
left a comment
There was a problem hiding this comment.
Общий вывод по проекту
Проект демонстрирует хорошее понимание архитектурных паттернов, таких как Front Controller и Command. Структура кода логична, а разделение на слои (entity, repository, service, controller) выполнено в соответствии с лучшими практиками. Однако стоит обратить внимание на потокобезопасность коллекций в многопоточной среде сервлетов и избегать «магических» чисел в тестах.
Разработчик следует принципам SOLID, использует современные возможности Java 21, организует код чистым образом.
Рекомендации:
- Продолжить углубление знаний по паттернам проектирования
- Изучить асинхронное программирование более глубоко
- Уделить внимание документированию кода
Итоговая оценка: A
| */ | ||
| @Getter | ||
| @Setter | ||
| public class GameSession { |
There was a problem hiding this comment.
Класс GameSession является изменяемым (mutable). Для хранения состояния сессии в многопоточной среде сервлетов следует обеспечить потокобезопасность или использовать неизменяемые структуры данных. [WARNING]
| public class QuestRepository { | ||
|
|
||
| /** Все шаги квеста: ключ — id шага, значение — объект QuestStep */ | ||
| private final Map<Integer, QuestStep> steps = new HashMap<>(); |
There was a problem hiding this comment.
Использование HashMap в многопоточной среде без внешней синхронизации может привести к неопределенному поведению. Рекомендуется заменить на ConcurrentHashMap. [ERROR]
| * -> "Взять всех" => [8] WIN | ||
| * -> "Взять только капитана" => [9] LOSE | ||
| */ | ||
| private void initQuest() { |
There was a problem hiding this comment.
Метод initQuest() перегружен жестко закодированными данными. Рекомендуется вынести конфигурацию квеста во внешние файлы (JSON/YAML) для соблюдения принципа единственной ответственности. [WARNING]
| * Получить шаг по id. | ||
| */ | ||
| public Optional<QuestStep> findById(int id) { | ||
| return Optional.ofNullable(steps.get(id)); |
There was a problem hiding this comment.
Метод findById возвращает Optional, что хорошо, но вызывающий код в тестах часто использует .orElseThrow() без аргументов, что может скрыть причину ошибки. [INFO]
|
|
||
| private static final Logger log = LoggerFactory.getLogger(SessionService.class); | ||
|
|
||
| public static final String SESSION_KEY = "gameSession"; |
There was a problem hiding this comment.
Ключ сессии 'gameSession' лучше вынести в общую константу или конфигурацию, чтобы избежать магических строк в разных частях приложения. [INFO]
| Optional<QuestStep> optional = questRepository.findById(i); | ||
| if (optional.isPresent()) { | ||
| QuestStep step = optional.get(); | ||
| if (step.isCompleted()) { |
There was a problem hiding this comment.
В цикле используется проверка 'step.isCompleted()', но сам тест называется finalStepsShouldHaveNoOptions. Проверка должна быть более явной относительно статусов WIN/LOSE. [INFO]
|
|
||
| public int getNextStepId(int currentStepId, String chosenOption) { | ||
| QuestStep currentStep = getStep(currentStepId); | ||
| Integer nextStepId = currentStep.getOptions().get(chosenOption); |
There was a problem hiding this comment.
Возможен NullPointerException, если currentStep.getOptions() вернет null. Хотя Builder гарантирует инициализацию, явная проверка добавит надежности. [WARNING]
| /** | ||
| * Кастомное исключение — бросается когда шаг квеста не найден. | ||
| */ | ||
| public class QuestStepNotFoundException extends RuntimeException { |
There was a problem hiding this comment.
Исключение является RuntimeException, что корректно. Однако стоит добавить serialVersionUID для соблюдения стандартов сериализации Java объектов. [INFO]
| GameSession gameSession = sessionService.getSession(request.getSession()); | ||
|
|
||
| // Если сессии нет — игрок не начинал игру, отправляем на старт | ||
| if (gameSession == null) { |
There was a problem hiding this comment.
Если сессия отсутствует, происходит редирект на '/'. Стоит убедиться, что StartPageCommand корректно обрабатывает такие случаи без бесконечных циклов. [INFO]
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| questRepository = new QuestRepository(); |
There was a problem hiding this comment.
Инициализация репозитория в каждом тесте — это правильно для изоляции. Однако сам репозиторий заполняется данными в конструкторе, что замедляет тесты. [INFO]
3 participants
No description provided.