Buslovskii_Quest#18
Conversation
demologin
left a comment
demologin
left a comment
There was a problem hiding this comment.
Общий вывод по проекту
Проект представляет собой классическую реализацию веб-приложения на сервлетах с применением паттерна Front Controller. Структура кода понятна, разделение на пакеты логично, а наличие тестов свидетельствует о хорошем подходе к качеству. Основные замечания касаются архитектурной гибкости (жесткая привязка данных в коде), потокобезопасности при работе с общим состоянием в слушателях и использования устаревших способов логирования. Устранение «магических чисел» и переход на внешнюю конфигурацию квестов значительно повысят уровень проекта.
Проделанная работа впечатляет: структура команд и навигация по квестам реализованы очень аккуратно. Это отличная база для перехода к изучению Spring Framework, где многие из указанных замечаний решаются встроенными механизмами. Продолжайте развивать навыки проектирования, и архитектурное мастерство придет с практикой!
Итоговая оценка: A
| @Override | ||
| public void sessionCreated(HttpSessionEvent se) { | ||
| activeSessions++; | ||
| System.out.println("Session created. Active sessions: " + activeSessions); |
There was a problem hiding this comment.
Использование System.out.println для логирования событий жизненного цикла сессии недопустимо в профессиональной разработке. Следует использовать специализированные фреймворки, такие как SLF4J с реализацией Logback, чтобы обеспечить гибкое управление уровнями логирования и форматом вывода. [WARNING]
| @WebListener | ||
| public class SessionListener implements HttpSessionListener { | ||
|
|
||
| private static int activeSessions = 0; |
There was a problem hiding this comment.
Поле activeSessions объявлено как статическое интовое значение без обеспечения потокобезопасности. В многопоточной среде сервлет-контейнера инкремент (++) и декремент (--) не являются атомарными операциями, что приведет к некорректному подсчету сессий. Рекомендуется использовать AtomicInteger. [ERROR]
| request.getRequestDispatcher(view).forward(request, response); | ||
| } | ||
| } catch (Exception e) { | ||
| throw new ServletException("Error executing command", e); |
There was a problem hiding this comment.
Перехват общего исключения Exception и его оборачивание в ServletException без предварительного логирования затрудняет отладку. Рекомендуется логировать стек вызовов перед пробросом исключения дальше. [INFO]
| } | ||
| } | ||
|
|
||
| private String getCommandPath(HttpServletRequest request) { |
There was a problem hiding this comment.
Метод getCommandPath содержит избыточную логику парсинга URI с ручным поиском символов '?', '#' и '.'. Современные методы API HttpServletRequest уже предоставляют разделенные части пути, а логика очистки расширений файлов может быть вынесена в отдельный утилитный класс. [INFO]
| } | ||
|
|
||
| private void initializeQuestions() { | ||
| questions = new HashMap<>(); |
There was a problem hiding this comment.
Инициализация вопросов внутри конструктора класса жестко связывает логику квеста с данными. Это нарушает принцип единственной ответственности (SRP). Рекомендуется вынести конфигурацию вопросов во внешние файлы (JSON/XML) или использовать паттерн 'Фабрика' для загрузки данных. [WARNING]
| private void initializeQuestions() { | ||
| questions = new HashMap<>(); | ||
|
|
||
| questions.put(1, new Question(1, |
There was a problem hiding this comment.
Тексты вопросов содержат многострочные литералы, смешанные с логикой кода. Это затрудняет локализацию приложения в будущем. Рекомендуется использовать ResourceBundle для хранения строковых ресурсов. [WARNING]
| System.out.println("Session destroyed. Active sessions: " + activeSessions); | ||
| } | ||
|
|
||
| public static int getActiveSessions() { |
There was a problem hiding this comment.
Метод getActiveSessions является статическим. В контексте веб-приложения доступ к глобальному состоянию через статику — это антипаттерн, затрудняющий масштабирование и изоляцию данных приложения. [WARNING]
| "Начать заново", | ||
| "Выбрать другой квест", | ||
| 1, 1, true, | ||
| null, |
There was a problem hiding this comment.
Передача null в конструктор Question для параметров victoryMessage/defeatMessage требует внимательной проверки на стороне получателя, чтобы избежать NullPointerException. Лучше использовать пустые строки или Optional. [INFO]
| import java.io.IOException; | ||
|
|
||
| @WebServlet("/") | ||
| public class FrontControllerServlet extends HttpServlet { |
There was a problem hiding this comment.
Название класса FrontControllerServlet корректно отражает паттерн, однако логика обработки команд могла бы быть более гибкой при использовании маппинга через аннотации или конфигурационные файлы вместо ручного парсинга строк. [INFO]
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| class QuestTest { |
There was a problem hiding this comment.
Класс теста и методы не имеют модификатора доступа public, что допустимо в JUnit 5, однако стоит придерживаться единообразия во всем проекте относительно видимости тестовых компонентов. [INFO]
2 participants
No description provided.