Conversation
Fix StartServlet.java Created and tested the logic of switching to a question with the mechanics of choosing an answer and checkin
…actoring StartServlet.java, QuestionServlet.java
fix Servlets
…ava, questions.json. fix Question.java, StartServlet.java, QuestionRepository.java
…test topics in the UI have been changed.
…java. Fixed StartServlet.java and JSONs questions
Added user logic
…statistics.jsp, TopicStats.java, UserTopicStats.java, UserTopicStats.java
…ter, config, repository
Общий вывод по проектуПроект демонстрирует уверенное владение стеком технологий: сервлеты настроены корректно, логика репозиториев отделена от тестов, а использование Mockito в тестах сервисов показывает понимание принципов модульного тестирования. Особенно хочется отметить грамотную реализацию обработки ошибок через кастомные исключения в пакете exception. Основные рекомендации: Типизация: Старайтесь уходить от строк там, где можно применить Enum (например, в причинах ошибок). Безопасность сессий: Не забывайте про интерфейс Serializable для объектов, хранящихся в сессии, и используйте session.invalidate() при выходе. Чистота тестов: Используйте современные возможности JUnit 5, такие как assertThrows и пакетная видимость классов. Ваш код написан аккуратно и логично. Исправление указанных архитектурных нюансов позволит перевести проект на уровень промышленной разработки. Удачи в дальнейшем изучении Java! Итоговая оценка: A |
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| public class QuestionRepositoryTest { |
There was a problem hiding this comment.
Класс теста объявлен как public. Начиная с JUnit 5, для тестовых классов и методов рекомендуется использовать пакетную видимость (package-private), чтобы не расширять область доступа без необходимости.
| Topic topic = Topic.JAVA_CORE; | ||
| List<Question> expectedQuestions = List.of( | ||
| mock(Question.class), | ||
| mock(Question.class) |
There was a problem hiding this comment.
Использование mock(Question.class) для простых POJO-моделей является избыточным. Для объектов данных (Data Objects) лучше создавать реальные экземпляры, чтобы тесты были более наглядными и менее зависимыми от внутренней реализации Mockito.
|
|
||
| private User createUser(String username) { | ||
| return new User( | ||
| 0L, |
There was a problem hiding this comment.
Использование магического числа '0L' при создании пользователя снижает читаемость. Рекомендуется использовать константу или вовсе не передавать ID, если он должен генерироваться автоматически репозиторием.
|
|
||
| repository.save(user); | ||
|
|
||
| assertTrue(user.getId() > 0); |
There was a problem hiding this comment.
Проверка assertTrue(user.getId() > 0) может быть нестабильной, если логика генерации ID изменится. Более надежно проверять, что ID не равен null (если это Long) или начальному значению.
| User user = createUser("john"); | ||
| repository.save(user); | ||
|
|
||
| Optional<User> found = repository.findByUserName("john"); |
There was a problem hiding this comment.
Нарушено единообразие именования методов. В Java принято camelCase: findByUsername вместо findByUserName (разница в заглавной 'N').
| 10, | ||
| 7, | ||
| true, | ||
| LocalDateTime.now() |
There was a problem hiding this comment.
Использование LocalDateTime.now() в тестах может привести к их нестабильности («мигающие» тесты). Лучше передавать фиксированный момент времени (Clock), чтобы результаты всегда были предсказуемы.
| } | ||
|
|
||
| public static InterviewState getInterviewState(HttpSession session) { | ||
| return (InterviewState) session.getAttribute(INTERVIEW_STATE); |
There was a problem hiding this comment.
Прямое приведение типа (InterviewState) без предварительной проверки instanceOf или использования Optional может привести к ClassCastException, если в сессии под этим ключом окажется другой объект.
| } | ||
|
|
||
| public static void clearInterview(HttpSession session) { | ||
| session.removeAttribute(INTERVIEW_STATE); |
There was a problem hiding this comment.
Метод clearInterview только удаляет атрибут. Если состояние интервью является критическим для безопасности сессии, стоит рассмотреть возможность полной инвалидации сессии в определенных сценариях.
|
|
||
| public class AuthenticationException extends RuntimeException { | ||
|
|
||
| private final String reason; |
There was a problem hiding this comment.
Поле reason объявлено как String. Для типизации ошибок аутентификации лучше использовать перечисления (Enum), чтобы избежать опечаток в строковых сообщениях по всему приложению.
|
|
||
| public static ValidationException email(String message) { | ||
| return new ValidationException(message, "email", "EMAIL_INVALID"); | ||
| } |
There was a problem hiding this comment.
Статические фабричные методы — это отличная практика, упрощающая создание исключений с конкретным контекстом ошибки.
| String topic = rawTopic.trim(); | ||
|
|
||
| TopicStats stats = | ||
| topicStats.computeIfAbsent(topic, TopicStats::new); |
There was a problem hiding this comment.
Использование computeIfAbsent — эффективный способ работы с картами (Map). Это демонстрирует хорошее владение современным Java API.
| req.setAttribute("userStats", userStats.values()); | ||
| req.setAttribute("topicStats", topicStats.values()); | ||
|
|
||
| req.getRequestDispatcher("/WEB-INF/jsp/admin/statistics.jsp") |
There was a problem hiding this comment.
Путь к JSP прописан жестко в коде. В больших проектах рекомендуется выносить префиксы и суффиксы путей в конфигурацию, чтобы избежать дублирования строк /WEB-INF/jsp/.
| @@ -0,0 +1,50 @@ | |||
| package com.javarush.zyibin.model; | |||
There was a problem hiding this comment.
Класс состояния должен реализовывать интерфейс Serializable, так как он сохраняется в HttpSession. В распределенных системах или при перезагрузке сервера сессии могут сериализоваться.
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.util.List; |
There was a problem hiding this comment.
Отсутствует механизм кэширования вопросов. Если QuestionSource обращается к файловой системе или внешнему API при каждом вызове, это может создать избыточную нагрузку.
| @Test | ||
| void shouldInitializeWithZeroCounters() { | ||
|
|
||
| UserTopicStats stats = new UserTopicStats("Java Core"); |
There was a problem hiding this comment.
Метод getSuccessRate() может привести к делению на ноль, если total равен 0. Необходимо добавить проверку внутри DTO или возвращать 0 по умолчанию.
| log.debug("Loading available avatars list"); | ||
| return List.of( | ||
| "/resources/avatars/default/avatar1.png", | ||
| "/resources/avatars/default/avatar2.png", |
There was a problem hiding this comment.
Список аватаров жестко задан в коде. Стоит рассмотреть возможность сканирования директории с ресурсами, чтобы добавление нового аватара не требовало перекомпиляции кода.
|
|
||
| @Override | ||
| public Optional<User> findById(long id) { | ||
| Optional<User> user = Optional.ofNullable(usersById.get(id)); |
There was a problem hiding this comment.
Метод findAll() возвращает прямой доступ к коллекции пользователей. В целях инкапсуляции и безопасности лучше возвращать Collections.unmodifiableList или копию списка.
| "Пароль должен храниться в захешированном виде" | ||
| ); | ||
|
|
||
| verify(userRepository, times(1)).save(any(User.class)); |
There was a problem hiding this comment.
Использование any(User.class) в verify — это слишком обобщенная проверка. Лучше проверять конкретные поля объекта, который был передан на сохранение, используя ArgumentCaptor.
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| class InMemoryUserRepositoryTest { |
There was a problem hiding this comment.
Тестовый класс не имеет модификатора доступа. Это правильное применение правил инкапсуляции в контексте JUnit 5.
| public class LogoutServlet extends BaseServlet { | ||
|
|
||
| @Override | ||
| protected void initializeSpecificServices() { |
There was a problem hiding this comment.
При выходе из системы (logout) рекомендуется всегда инвалидировать сессию полностью (session.invalidate()), а не просто удалять отдельные атрибуты.
| @@ -0,0 +1,11 @@ | |||
| package com.javarush.zyibin.source; | |||
There was a problem hiding this comment.
Интерфейс не содержит комментариев о том, какие исключения может выбрасывать метод loadQuestions (например, если источник данных недоступен).
|
|
||
| import java.time.LocalDateTime; | ||
|
|
||
|
|
There was a problem hiding this comment.
Поля класса User (username, email) должны иметь валидацию на уровне конструктора или сеттеров, чтобы предотвратить создание объектов с некорректными данными.
No description provided.