Conversation
Implemented MVC with Servlets and JSP. Added HttpSession for player tracking (name/counter). Integrated QuestRepository and GameService. Configured AppContextListener for data init.
demologin
left a comment
demologin
left a comment
There was a problem hiding this comment.
Общий вывод по проекту
Проделана отличная работа по созданию архитектуры квеста. Видно, что вы хорошо разбираетесь в разделении ответственности на уровни (Entity, Repository, Service, Controller) и умеете использовать современные библиотеки вроде Lombok и Mockito. Это крепкий фундамент для профессионального роста.
Проект выполнен на очень достойном уровне. Архитектура логична, код легко читается, и вы успешно применили паттерн Repository и Service. Основные моменты для улучшения связаны с обработкой исключений, потокобезопасностью коллекций в многопоточной среде сервлетов и уходом от проверки бизнес-логики через строковые значения (заголовки шагов).
Рекомендации:
- Продолжить углубление знаний по паттернам проектирования
- Изучить асинхронное программирование более глубоко
- Уделить внимание документированию кода
Итоговая оценка: A
|
|
||
| public class QuestRepository implements Repository<QuestStep> { | ||
| private static final Logger logger = LogManager.getLogger(QuestRepository.class); | ||
| private final Map<Integer, QuestStep> steps = new HashMap<>(); |
There was a problem hiding this comment.
Использование HashMap не обеспечивает потокобезопасность. В многопоточном окружении сервлетов лучше использовать ConcurrentHashMap для хранения шагов квеста. [WARNING]
| @Override | ||
| public void load() { | ||
| logger.info("Начало загрузки конфигурационных файлов квеста..."); | ||
| YAMLMapper mapper = new YAMLMapper(); |
There was a problem hiding this comment.
Экземпляр YAMLMapper можно вынести в статическое поле (константу), так как он потокобезопасен и его переиспользование эффективнее создания нового объекта при каждой загрузке. [INFO]
|
|
||
| logger.info("Успешно загружено шагов: {} из файла: {}", loaded.size(), fileName); | ||
|
|
||
| } catch (Exception e) { |
There was a problem hiding this comment.
Перехват общего исключения Exception — плохая практика. Рекомендуется перехватывать конкретные проверяемые исключения (например, IOException), чтобы не скрыть непредвиденные ошибки рантайма. [WARNING]
| } | ||
|
|
||
| String title = step.getTitle().toLowerCase(); | ||
| return title.contains("победа") || |
There was a problem hiding this comment.
Логика завершения игры завязана на текстовые значения заголовка ('победа', 'поражение'). Это нарушает принцип инверсии зависимостей и делает код хрупким. Лучше добавить в QuestStep булево поле isFinalStep. [ERROR]
|
|
||
| @Override | ||
| public boolean isGameOver(QuestStep step) { | ||
| if (step == null || step.getTitle() == null) { |
There was a problem hiding this comment.
Метод содержит избыточную проверку. Если step == null, метод упадет на step.getTitle() раньше, чем дойдет до логики внутри условия. Стоит использовать Optional или более строгую проверку в начале. [WARNING]
| public class AppContextListener implements ServletContextListener { | ||
| @Override | ||
| public void contextInitialized(ServletContextEvent sce) { | ||
| QuestRepository repository = new QuestRepository(); |
There was a problem hiding this comment.
Инициализация QuestRepository происходит в блоке инициализации контекста. Если загрузка файлов упадет, приложение не запустится. Стоит добавить try-catch для обработки ошибок при старте. [ERROR]
|
|
||
| import java.io.IOException; | ||
|
|
||
| @WebServlet(name = "InitServlet", value = "/init") |
There was a problem hiding this comment.
Имя сервлета 'InitServlet' достаточно общее. Согласно Clean Code, лучше давать имена, отражающие бизнес-суть, например 'PlayerRegistrationServlet'. [INFO]
| return step != null && (step.getAnswers() == null || step.getAnswers().isEmpty()); | ||
| } | ||
|
|
||
| String title = step.getTitle().toLowerCase(); |
There was a problem hiding this comment.
Метод toLowerCase() без указания Locale может работать некорректно на некоторых системах (например, с турецким языком). Используйте Locale.ROOT. [INFO]
| } | ||
|
|
||
| List<QuestStep> loaded = mapper.readValue(is, new TypeReference<>() {}); | ||
| for (QuestStep step : loaded) { |
There was a problem hiding this comment.
Если в разных YAML файлах встретятся шаги с одинаковыми ID, они будут перезаписаны без предупреждения. Стоит добавить проверку на уникальность ID. [WARNING]
| void shouldCreatePlayerAndIncrementGames() { | ||
| Player player = new Player("Герой", "static/images/avatars/1.png"); | ||
|
|
||
| assertAll("Инициализация игрока", |
There was a problem hiding this comment.
Использование assertAll — отличная практика для группировки проверок состояния одного объекта. Код написан чисто и понятно. [INFO]
2 participants
No description provided.