Bakhtin Denis Project Quest#20
Conversation
demologin
left a comment
demologin
left a comment
There was a problem hiding this comment.
Общий вывод по проекту
Проект демонстрирует хорошее понимание основ Java Servlets и паттерна Builder. Структура исключений продумана, а использование фильтра для обработки ошибок — это отличное архитектурное решение. Однако, следует уделить больше внимания безопасности типов (приведение атрибутов сессии) и инкапсуляции данных. Использование статических генераторов ID может создать проблемы при масштабировании приложения, поэтому стоит рассмотреть более надежные способы идентификации сущностей.
Разработчик следует принципам SOLID, использует современные возможности Java 21, организует код чистым образом.
Рекомендации:
- Продолжить углубление знаний по паттернам проектирования
- Изучить асинхронное программирование более глубоко
- Уделить внимание документированию кода
Итоговая оценка: A
| @Setter | ||
| Question startQuestion; | ||
| String name; | ||
| ConcurrentHashMap<Long, Question> questions; |
There was a problem hiding this comment.
Использование ConcurrentHashMap в классе, помеченном как Serializable, может быть избыточным, если не предполагается многопоточный доступ к состоянию квеста в рамках одной сессии. [INFO]
| private static final long serialVersionUID = 1L; | ||
| private boolean finish; | ||
| private boolean win; | ||
| private static final AtomicLong idGenerator = new AtomicLong(0); |
There was a problem hiding this comment.
Статическое поле idGenerator в нестатическом внутреннем классе Question приведет к сквозной нумерации ID между всеми экземплярами квестов. Рекомендуется пересмотреть логику генерации ID для обеспечения изоляции данных. [WARNING]
| private boolean finish; | ||
| private boolean win; | ||
| private static final AtomicLong idGenerator = new AtomicLong(0); | ||
| String question; |
There was a problem hiding this comment.
Поле 'question' имеет уровень доступа по умолчанию (package-private). Согласно принципам инкапсуляции, следует использовать модификатор private. [WARNING]
| protected void doPost(HttpServletRequest req, HttpServletResponse resp) | ||
| throws ServletException, IOException { | ||
|
|
||
| String actionStr = req.getParameter(SessionConstants.ACTION_PARAM); |
There was a problem hiding this comment.
Параметр 'action' извлекается без проверки на null перед передачей в Action.parse. Это может привести к исключению в логике парсинга. [WARNING]
| private void doExit(HttpServletRequest req, HttpServletResponse resp, | ||
| HttpSession session) throws ServletException, IOException { | ||
| session.invalidate(); | ||
| req.getRequestDispatcher("/WEB-INF/hello.jsp").forward(req, resp); |
There was a problem hiding this comment.
Использование forward после invalidate() сессии может привести к непредсказуемому поведению, если JSP ожидает наличие сессионных данных. Лучше использовать redirect. [WARNING]
| @Getter | ||
| public static class Answer implements Serializable { | ||
|
|
||
| private static final long serialVersionUID = 1L; |
There was a problem hiding this comment.
serialVersionUID установлен вручную в 1L. Это хорошая практика для Serializable, но стоит убедиться, что все вложенные классы также следуют этому правилу консистентно. [INFO]
| class QuestTest { | ||
|
|
||
| @Test | ||
| void testBuilderName() { |
There was a problem hiding this comment.
Тесты проверяют только успешные сценарии (Happy Path). Отсутствуют тесты на некорректные данные или граничные условия билдеров. [WARNING]
|
|
||
| private String name; | ||
| private Question startQuestion; | ||
| private final Map<Long, Question> questions = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Поле 'questions' в QuestBuilder помечено как final, что ограничивает возможность сброса состояния билдера при повторном использовании. [INFO]
|
|
||
| Question nextQuestion = getNextQuestion(question, id); | ||
|
|
||
| if (nextQuestion.isFinish()) { |
There was a problem hiding this comment.
Логика завершения игры (doFinish) смешана с логикой обработки ответа (doAnswer). Следует разделить ответственность согласно принципу Single Responsibility. [WARNING]
|
|
||
| public Question addNewQuestion(String question, List<Answer> answers) { | ||
| Question q = new Question(question, answers); | ||
| if (q.getId().equals(1L)) { |
There was a problem hiding this comment.
Магическое число '1L' для определения стартового вопроса. Лучше использовать явный флаг или передавать стартовый вопрос в конструктор Quest. [WARNING]
2 participants
No description provided.