Conversation
Fixed the display of the Admin Panel. No tests.
Moved the logic of the JS into admin.js.
No test.
It's still a bit clunky.
Fixed a bug with an empty password. Found a bug with quest selection during a quest. Needs fixing.
The project is ready.
Общий вывод по проектуПроект демонстрирует хорошее владение базовыми принципами Java и архитектурой веб-приложений на базе сервлетов. Видно стремление к структурированию кода: выделены слои репозиториев, сервисов и моделей, что соответствует классическим паттернам. Использование современных текстовых блоков в тестах и Java 21 фич (хотя и не в полной мере) говорит о прогрессивном подходе к обучению. Основные точки роста: Инверсия управления (IoC): Сейчас проект сильно опирается на статические методы и глобальные состояния (особенно в репозиториях). Переход к созданию экземпляров классов и их внедрению через конструкторы сделает код гибким и профессиональным. Обработка исключений и логирование: Замена System.out.println на полноценный логгер и использование Optional вместо возврата null значительно повысит надежность приложения. Безопасность и валидация: Стоит уделить внимание защите данных (пароли) и проверке того, что приходит от пользователя в сервлеты. Итоговая оценка: A |
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| class QuestRepositoryTest { |
There was a problem hiding this comment.
Использование модификатора public в тестовых классах JUnit 5 избыточно. Пакетная видимость является предпочтительной для чистоты кода.
|
|
||
| @BeforeEach | ||
| void clearRepositoryBeforeEachTest() { | ||
| QuestRepository.clear(); |
There was a problem hiding this comment.
Метод QuestRepository.clear() намекает на наличие глобального состояния. Для тестов лучше использовать внедрение зависимостей (Dependency Injection), чтобы изолировать тесты друг от друга.
| @DisplayName("Test loadTxt stores quest and get returns it") | ||
| void testLoadTxtStoresQuestAndGetReturnsIt() throws Exception { | ||
|
|
||
| String data = """ |
There was a problem hiding this comment.
Текстовые блоки (Text Blocks) — отличная фича Java 15+. Стоит убедиться, что отступы внутри блока соответствуют ожидаемому формату парсера.
|
|
||
| ServletContext ctx = mock(ServletContext.class); | ||
|
|
||
| when(ctx.getResourcePaths("/WEB-INF/classes/quests/")) |
There was a problem hiding this comment.
Использование магических строк вроде '/WEB-INF/classes/quests/' дублируется в тестах. Рекомендуется вынести их в константы для соблюдения принципа DRY.
|
|
||
| import com.javarush.golikov.quest.auth.Role; | ||
| import com.javarush.golikov.quest.model.User; | ||
| import org.junit.jupiter.api.*; |
There was a problem hiding this comment.
Использование импортов со звездочкой (wildcard imports) затрудняет чтение зависимостей. Лучше импортировать конкретные классы: @test, @beforeeach и т.д.
| import com.javarush.golikov.quest.repository.UserRepository; | ||
| import jakarta.servlet.http.HttpSession; | ||
|
|
||
| public class AuthService { |
There was a problem hiding this comment.
Класс не помечен как final. Если наследование не предполагается, хорошей практикой является закрытие класса для изменений.
|
|
||
| import java.util.Map; | ||
|
|
||
| public class Quest { |
There was a problem hiding this comment.
Отсутствует конструктор по умолчанию или конструктор со всеми полями. Стоит явно определить инициализацию объекта.
| public User login(String login, String password) { | ||
| User user = UserRepository.find(login); | ||
| if (user != null && user.password().equals(password)) { | ||
| return user; |
There was a problem hiding this comment.
Пароли хранятся или передаются в открытом виде. С точки зрения безопасности, это критическая уязвимость.
| Choice choice = node.choices().stream() | ||
| .filter(c -> c.next().equals(next)) | ||
| .findFirst() | ||
| .orElseThrow(); |
There was a problem hiding this comment.
Метод возвращает null в случае ошибки. Лучше использовать Optional или выбрасывать кастомное исключение.
|
|
||
| try (MockedStatic<UserRepository> repoMock = mockStatic(UserRepository.class)) { | ||
| repoMock.when(() -> UserRepository.find("admin")) | ||
| .thenReturn(user); |
There was a problem hiding this comment.
Тест проверяет сразу несколько сценариев (успешный вход и неверный пароль). Лучше разделить их на разные тестовые методы.
| @Test | ||
| void testRemoveUnknownIdDoesNothing() { | ||
| QuestRepository.remove("unknown"); | ||
| assertTrue(QuestRepository.all().isEmpty()); |
There was a problem hiding this comment.
Ассерты в тестах не содержат описания ошибки. Добавление сообщения в assertEquals поможет быстрее понять причину падения теста.
Pingbot |
3 participants
No description provided.