Conversation
Сhanged visualization, added background
…, statistics added
Общий вывод по проектуПроект демонстрирует хорошее понимание основ сервлетов и архитектуры Command. Однако, стоит уделить больше внимания безопасности (валидация входных данных) и чистоте кода (избавление от магических строк и вложенной тернарной логики). Тесты покрывают основные сценарии, но местами опираются на хрупкие проверки строковых представлений. Рекомендации:
Итоговая оценка: A |
| public String doPost(HttpServletRequest request) { | ||
| if (request.getParameter("logout") == null) { | ||
| HttpSession session = request.getSession(); | ||
| User user = (User) session.getAttribute("user"); |
There was a problem hiding this comment.
Использование 'магических строк' для атрибутов сессии. Рекомендуется использовать константы из класса Key для поддержания единообразия.
| if (request.getParameter("logout") == null) { | ||
| HttpSession session = request.getSession(); | ||
| User user = (User) session.getAttribute("user"); | ||
| return Go.EDIT_USER + "?id=" + user.getId(); |
There was a problem hiding this comment.
Отсутствует проверка на null для объекта user перед вызовом getId(), что может привести к NullPointerException.
| @Override | ||
| public String doGet(HttpServletRequest req) { | ||
| req.setAttribute(QUESTS, questService.getAll()); | ||
| return getView(); |
There was a problem hiding this comment.
Лишний пустой метод или дублирование логики базового интерфейса. Стоит проверить необходимость переопределения, если логика стандартна.
|
|
||
| @Override | ||
| public String doGet(HttpServletRequest request) { | ||
| Long questId = Long.parseLong(request.getParameter(Key.QUEST_ID)); |
There was a problem hiding this comment.
Прямое парсинг параметра без проверки на null или формат числа. Может вызвать NumberFormatException.
| } else { | ||
| String message = "Нет незавершенной игры"; | ||
| log.warn(message); | ||
| RequestHelpers.createError(request, message); |
There was a problem hiding this comment.
Дублирование логики создания ошибок. Рекомендуется вынести обработку исключительных ситуаций в глобальный фильтр или контроллер.
| private void showOneQuestion(HttpServletRequest request, Game game) { | ||
| request.setAttribute(Key.GAME, game); | ||
| Optional<Question> question = questionService.get(game.getCurrentQuestionId()); | ||
| request.setAttribute(Key.QUESTION, question.orElseThrow()); |
There was a problem hiding this comment.
Вызов orElseThrow() без указания конкретного исключения. Лучше выбрасывать кастомное исключение с понятным описанием.
| public String doGet(HttpServletRequest req) { | ||
| String stringId = req.getParameter(Key.ID); | ||
| if (stringId != null) { | ||
| long id = Long.parseLong(stringId); |
There was a problem hiding this comment.
Небезопасное приведение строки к long. Отсутствует обработка ошибок ввода (валидация ID).
| String roleParam = req.getParameter("role"); | ||
| String genderParam = req.getParameter("gender"); | ||
| Role role = roleParam != null | ||
| ? Role.valueOf(roleParam) |
There was a problem hiding this comment.
Использование тернарных операторов такой вложенности снижает читаемость. Рекомендуется использовать методы-хелперы.
| } | ||
| String imageId = "image-" + user.getId(); | ||
| Part imagePart = req.getPart("image"); | ||
| if (imagePart != null && imagePart.getInputStream().available() > 0) { |
There was a problem hiding this comment.
Логика проверки наличия стрима через available() может быть ненадежной для всех типов Part. Лучше проверять размер или имя файла.
|
|
||
| class ProfileIT extends BaseIT { | ||
|
|
||
| private final Profile profile = Winter.find(Profile.class); |
There was a problem hiding this comment.
Поле profile не инициализируется через Mockito или конструктор, а напрямую через Winter. Это усложняет изоляцию тестов.
|
|
||
| String uri = signup.doPost(request); | ||
| Assertions.assertEquals(Go.PROFILE, uri); | ||
| Assertions.assertTrue(repository.getAll().toString().contains("newTestLogin")); |
There was a problem hiding this comment.
Проверка содержания строки через toString().contains() является хрупкой. Лучше проверять наличие объекта в коллекции через equals/id.
| @Test | ||
| @DisplayName("When login admin then redirect to profile") | ||
| void whenLoginAdminThenRedirectToProfile() { | ||
| User user = userService.getAll().stream().findFirst().orElseThrow(); |
There was a problem hiding this comment.
Использование stream().findFirst().orElseThrow() в тестах без предварительной подготовки данных может привести к нестабильности.
| User admin = User.builder().id(1L).role(Role.ADMIN).build(); | ||
| when(session.getAttribute(Key.USER)).thenReturn(admin); | ||
| when(request.getParameter(Key.NAME)).thenReturn("TestQuest"); | ||
| when(request.getParameter(Key.TEXT)).thenReturn("1: Test OK?\n2< Да\n3< Нет\n2+ win\n3- lost\n"); |
There was a problem hiding this comment.
Хардкод тестовых данных (текст квеста) прямо в коде теста. Рекомендуется выносить такие блоки в ресурсы или константы.
|
|
||
| class UserRepositoryTest { | ||
|
|
||
| private final UserRepository userRepository = new UserRepository(); |
There was a problem hiding this comment.
Репозиторий инициализируется вручную через new, что противоречит общему стилю использования DI (Winter).
| import com.javarush.popkov.util.Key; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.Part; | ||
| import lombok.SneakyThrows; |
There was a problem hiding this comment.
Использование @SneakyThrows скрывает проверяемые исключения, что затрудняет отладку и понимание логики обработки ошибок.
|
|
||
| @SuppressWarnings("unused") | ||
| @Slf4j | ||
| public class PlayGame implements Command { |
There was a problem hiding this comment.
Класс помечен @slf4j, но логгирование используется точечно. Стоит добавить логирование критических переходов в игре.
| import jakarta.servlet.http.HttpSession; | ||
|
|
||
| @SuppressWarnings("unused") | ||
| public class Profile implements Command { |
There was a problem hiding this comment.
Класс Command должен быть максимально тонким. Логика определения переходов (Go.EDIT_USER) размазана по командам.
| protected User testUser; | ||
| protected User testGuest; | ||
|
|
||
| protected BaseIT() { |
There was a problem hiding this comment.
Конструктор базового класса выполняет слишком много работы (инициализация моков, данных). Лучше использовать @beforeeach.
| Mockito.when(request.getParameter(Key.QUESTION_ID)).thenReturn("1"); | ||
| Mockito.when(request.getParameter(Key.TEXT)).thenReturn("newTestTextQuestion"); | ||
| String actualUri = quest.doPost(request); | ||
| String expectedUri = "%s?id=%d#bookmark%d".formatted(Go.QUEST, 1, 1); |
There was a problem hiding this comment.
Сложное форматирование ожидаемого URI. Есть риск ошибки при изменении структуры ссылок.
| import static com.javarush.popkov.util.Key.QUESTS; | ||
|
|
||
| @SuppressWarnings("unused") | ||
| @AllArgsConstructor |
There was a problem hiding this comment.
Аннотация @AllArgsConstructor из Lombok удобна, но в Java 21 для DTO лучше использовать Records (хотя здесь это Command).
| ? Gender.valueOf(genderParam) | ||
| : existingUser != null ? existingUser.getGender() : Gender.MALE; | ||
| User user = User.builder() | ||
| .login(req.getParameter("login")) |
There was a problem hiding this comment.
Параметры 'login' и 'password' не проходят валидацию на пустые строки или длину.
|
|
||
| @Override | ||
| public String doPost(HttpServletRequest request) { | ||
| Long gameId = RequestHelpers.getId(request); |
There was a problem hiding this comment.
Использование статического хелпера RequestHelpers затрудняет мокирование и тестирование логики команд.
| when(request.getParameter("password")).thenReturn("err"); | ||
|
|
||
| String actualRedirect = login.doPost(request); | ||
| Assertions.assertEquals("/login", actualRedirect); |
There was a problem hiding this comment.
Проверка только URI редиректа. Стоит также проверять отсутствие атрибута USER в сессии при ошибке.
No description provided.