Skip to content

Matsarskaya#8

Open
KitaYaro wants to merge 14 commits intodemologin:mainfrom
KitaYaro:matsarskaya
Open

Matsarskaya#8
KitaYaro wants to merge 14 commits intodemologin:mainfrom
KitaYaro:matsarskaya

Conversation

@KitaYaro
Copy link

No description provided.

Copy link
Owner

@demologin demologin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Общий вывод по проекту

Проект демонстрирует хорошее владение инструментами тестирования (JUnit 5, Mockito, AssertJ) и понимание структуры веб-приложения на сервлетах. Код тестов структурирован, используются понятные названия методов и аннотации @DisplayName.

Основные зоны роста:

Обработка строк в исключениях: Исправьте синтаксические ошибки в конструкторах исключений (файлы UserNotFoundException.java).

Магические числа и строки: Проект перенасыщен литералами. Вынесение путей к JSP, ключей сессии и числовых констант в отдельные классы-константы (например, Attributes, Pages, GameConfig) значительно повысит читаемость.

Безопасность: Работа с паролями в формате String допустима для учебного проекта, но в реальных системах это критическая уязвимость. Это мы будем проходить на 5 модуле.

Архитектура: Постарайтесь больше использовать интерфейсы для сервисов и репозиториев, чтобы тесты не зависели от конкретных реализаций файлового хранилища.

Вы на правильном пути! Работа с файловой системой и интеграция ее в веб-приложение — отличная задача для закрепления Java Core.

Итоговая оценка: A


public class UserNotFoundException extends RuntimeException {
public UserNotFoundException(String username) {
super("The user named '\" username + \"' was not found");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Используется некорректная конкатенация строк в super(). Вместо '" username + "' следует использовать стандартную конкатенацию или String.format(). Это приведет к выводу литерала вместо значения. [ERROR]

}

public UserNotFoundException(String username, Throwable cause) {
super("The user named '\" username + \"' was not found", cause);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аналогичная ошибка в конструкторе с причиной (cause). Текст сообщения содержит синтаксическую ошибку в Java-строке. [ERROR]


@BeforeEach
void setUp() {
homePage = new HomePage();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Объект HomePage создается вручную через 'new'. Рекомендуется использовать Mockito для консистентности тестов, если класс имеет зависимости. [INFO]

@Test
@DisplayName("The beginning of the quest")
void testDoPostStartQuest() {
when(request.getParameter("quest")).thenReturn("the way of the dragon rider");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тесты содержат 'захардкоженные' строковые литералы. Рекомендуется вынести их в константы для облегчения поддержки кода. [WARNING]

String result = questDragon.doPost(request);

assertThat(result).isEqualTo("/WEB-INF/quest-dragon.jsp");
verify(session).setAttribute("stage", 0);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование магических чисел (0, 50). Следует использовать именованные константы, чтобы было понятно, что 50 — это начальный уровень доверия. [WARNING]

assertThat(result).isEqualTo("/WEB-INF/statistic-page.jsp");
verify(request).getSession();
verify(session).getAttribute("username");
verify(statisticService, never()).getStatistic(anyString());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование anyString() в verify — хорошая практика, но здесь было бы полезнее проверить, что метод НЕ вызывается именно для конкретного случая. [INFO]

void testGetUsersFilePath() {
String path = FileStorageConfig.getUsersFilePath();

assertThat(path).isEqualTo("users.txt");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тест завязан на конкретные имена файлов ('users.txt'). Если конфигурация изменится, тест упадет. Лучше проверять наличие значения, а не его контент. [WARNING]

String result = loginPage.doPost(request);

assertThat(result).isEqualTo("/WEB-INF/login-page.jsp");
verify(request).setAttribute("error", "User not found");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Метод setAttribute вызывается с жестко заданным ключом 'error'. Рекомендуется централизованное управление ключами атрибутов запроса. [INFO]


public class UserAlreadyExistsException extends RuntimeException {
public UserAlreadyExistsException(String username) {
super("A user named '\" username + \"' already exists");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сообщение об ошибке обрезано в сниппете, но вероятно также содержит конкатенацию. Проверьте единообразие формата сообщений исключений. [INFO]


assertThat(result).isEqualTo("/WEB-INF/quest-dragon.jsp");
verify(session).setAttribute("playerName", "PlayerName");
verify(session).setAttribute("stage", 2);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Переход между стадиями (1 -> 2) жестко прописан. Это делает логику квеста негибкой. Рассмотрите вынос логики переходов в отдельный конфиг. [WARNING]

@demologin
Copy link
Owner

Общий вывод по проекту

Проект демонстрирует хорошее владение инструментами тестирования (JUnit 5, Mockito, AssertJ) и понимание структуры веб-приложения на сервлетах. Код тестов структурирован, используются понятные названия методов и аннотации @DisplayName.

Основные зоны роста:

Обработка строк в исключениях: Исправьте синтаксические ошибки в конструкторах исключений (файлы UserNotFoundException.java).

Магические числа и строки: Проект перенасыщен литералами. Вынесение путей к JSP, ключей сессии и числовых констант в отдельные классы-константы (например, Attributes, Pages, GameConfig) значительно повысит читаемость.

Безопасность: Работа с паролями в формате String допустима для учебного проекта, но в реальных системах это критическая уязвимость. Это мы будем проходить на 5 модуле.

Архитектура: Постарайтесь больше использовать интерфейсы для сервисов и репозиториев, чтобы тесты не зависели от конкретных реализаций файлового хранилища.

Вы на правильном пути! Работа с файловой системой и интеграция ее в веб-приложение — отличная задача для закрепления Java Core.

Итоговая оценка: A


public class UserNotFoundException extends RuntimeException {
public UserNotFoundException(String username) {
super("The user named '\" username + \"' was not found");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Используется некорректная конкатенация строк в super(). Вместо '" username + "' следует использовать стандартную конкатенацию или String.format(). Это приведет к выводу литерала вместо значения.

}

public UserNotFoundException(String username, Throwable cause) {
super("The user named '\" username + \"' was not found", cause);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аналогичная ошибка в конструкторе с причиной (cause). Текст сообщения содержит синтаксическую ошибку в Java-строке.


@BeforeEach
void setUp() {
homePage = new HomePage();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Объект HomePage создается вручную через 'new'. Рекомендуется использовать Mockito для консистентности тестов, если класс имеет зависимости.

@Test
@DisplayName("The beginning of the quest")
void testDoPostStartQuest() {
when(request.getParameter("quest")).thenReturn("the way of the dragon rider");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тесты содержат 'захардкоженные' строковые литералы. Рекомендуется вынести их в константы для облегчения поддержки кода.

String result = questDragon.doPost(request);

assertThat(result).isEqualTo("/WEB-INF/quest-dragon.jsp");
verify(session).setAttribute("stage", 0);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование магических чисел (0, 50). Следует использовать именованные константы, чтобы было понятно, что 50 — это начальный уровень доверия.

@DisplayName("Defeat at a low level of trust")
void testDoPostLossCondition() {
when(request.getParameter("stage")).thenReturn("4");
when(request.getParameter("choice")).thenReturn("-10");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Значение '-10' передается как строка. Стоит проверить логику обработки отрицательных значений в сервисе, чтобы избежать NumberFormatException.


User user = new User("existinguser", "password123");

assertThatThrownBy(() -> repository.save(user))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В тесте проверяется только тип исключения и часть сообщения. Рекомендуется также проверять состояние моков (verify) после выброса исключения.

@DisplayName("Saving and uploading statistics")
void testSaveAndLoadStatistic() throws IOException {
testFilePath = tempDir.resolve("test_statistics.txt").toString();
System.setProperty("statistics.file.path", testFilePath);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прямая манипуляция системными свойствами (System.setProperty) в тестах может привести к побочным эффектам для других тестов. Лучше использовать конфигурационный файл или внедрение зависимости.

testFilePath = tempDir.resolve("test_statistics.txt").toString();
System.setProperty("statistics.file.path", testFilePath);

repository = new FileStatisticRepository();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Репозиторий создается в каждом тесте. Стоит вынести инициализацию в @beforeeach.

String result = registerPage.doPost(request);

assertThat(result).isEqualTo("/WEB-INF/register-page.jsp");
verify(request).setAttribute("error", "The user already exists");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Текст ошибки 'The user already exists' зашит прямо в коде. Для интернационализации (i18n) лучше использовать ResourceBundle.

import static org.mockito.Mockito.*;

@ExtendWith(MockitoExtension.class)
@DisplayName("Тесты для LoginPage")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Смешение языков в DisplayName: 'Тесты для LoginPage' (RU) и 'Tests for RegisterPage' (EN) в других файлах. Стоит придерживаться одного стиля.

when(request.getParameter("username")).thenReturn("testuser");
when(request.getParameter("password")).thenReturn("password123");
when(userService.loginUser("testuser", "password123"))
.thenReturn(Optional.of(new User("testuser", "password123")));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Создание 'new User' внутри when(). Для чистоты тестов лучше вынести создание сущностей в отдельные методы или переменные.

@Test
@DisplayName("The POST request performs a logout")
void testDoPost() {
doNothing().when(userService).logout(request);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование doNothing() для void методов Mockito является поведением по умолчанию. Этот вызов избыточен.

logger.info("User {} saved successfully", user.getUsername());
return true;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Класс зависит от конкретной реализации хранилища. Нарушение принципа инверсии зависимостей (DIP). Следует использовать интерфейсы.

@DisplayName("Moving to the next stage with a choice")
void testDoPostNextStage() {
when(request.getParameter("stage")).thenReturn("2");
when(request.getParameter("choice")).thenReturn("10");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Параметр 'choice' со значением '10' выглядит как 'магическое значение'. Неясно, как оно влияет на логику без заглядывания в реализацию.

String result = questDragon.doPost(request);

assertThat(result).isEqualTo("/WEB-INF/quest-dragon.jsp");
verify(statisticService).registerWin("testuser");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Метод registerWin вызывается с именем пользователя из сессии. Стоит добавить тест на случай, если username в сессии отсутствует (null).

@DisplayName("Verification of user's existence - user does not exist")
void testExistsByUsernameFalse() {
when(storage.userExists("nonexistent")).thenReturn(false);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тест проверяет логику репозитория, которая дублирует логику хранилища (storage). Возможно, репозиторий является лишним слоем в текущем виде.

void testDoGetWithStatistic() {
when(request.getSession()).thenReturn(session);
when(session.getAttribute("username")).thenReturn("testuser");
Statistic stat = new Statistic("testuser", 10, 5, 5);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Конструктор Statistic принимает много параметров. При расширении полей будет сложно поддерживать. Рассмотрите паттерн Builder.

void testDoPostSuccess() {
when(request.getParameter("username")).thenReturn("newuser");
when(request.getParameter("password")).thenReturn("password123");
doNothing().when(userService).registerUser("newuser", "password123");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пароли передаются в открытом виде (String). Это небезопасно. Рекомендуется использовать char[] или специализированные объекты для секретов.

assertThat(result).isEqualTo("/WEB-INF/quest-dragon.jsp");
verify(session).setAttribute("stage", 0);
verify(session).setAttribute("trust", 50);
verify(session).setAttribute("questFinished", false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Флаг 'questFinished' управляется строковыми ключами. Опечатка в ключе приведет к трудноуловимым багам. Используйте константы.

assertThat(result).isEqualTo("/WEB-INF/statistic-page.jsp");
verify(request).getSession();
verify(session).getAttribute("username");
verify(statisticService, never()).getStatistic(anyString());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Использование anyString() в verify — хорошая практика, но здесь было бы полезнее проверить, что метод НЕ вызывается именно для конкретного случая.

void testGetUsersFilePath() {
String path = FileStorageConfig.getUsersFilePath();

assertThat(path).isEqualTo("users.txt");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тест завязан на конкретные имена файлов ('users.txt'). Если конфигурация изменится, тест упадет. Лучше проверять наличие значения, а не его контент.

String result = loginPage.doPost(request);

assertThat(result).isEqualTo("/WEB-INF/login-page.jsp");
verify(request).setAttribute("error", "User not found");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Метод setAttribute вызывается с жестко заданным ключом 'error'. Рекомендуется централизованное управление ключами атрибутов запроса.


public class UserAlreadyExistsException extends RuntimeException {
public UserAlreadyExistsException(String username) {
super("A user named '\" username + \"' already exists");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сообщение об ошибке обрезано в сниппете, но вероятно также содержит конкатенацию. Проверьте единообразие формата сообщений исключений.


assertThat(result).isEqualTo("/WEB-INF/quest-dragon.jsp");
verify(session).setAttribute("playerName", "PlayerName");
verify(session).setAttribute("stage", 2);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Переход между стадиями (1 -> 2) жестко прописан. Это делает логику квеста негибкой. Рассмотрите вынос логики переходов в отдельный конфиг.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants