Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 0 additions & 37 deletions src/main/java/com/javarush/khmelov/cmd/Command.java

This file was deleted.

52 changes: 0 additions & 52 deletions src/main/java/com/javarush/khmelov/cmd/EditUser.java

This file was deleted.

26 changes: 0 additions & 26 deletions src/main/java/com/javarush/khmelov/cmd/ListUser.java

This file was deleted.

6 changes: 0 additions & 6 deletions src/main/java/com/javarush/khmelov/cmd/StartPage.java

This file was deleted.

9 changes: 9 additions & 0 deletions src/main/java/com/javarush/khmelov/config/StoryFiles.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package com.javarush.khmelov.config;

public class StoryFiles {
private StoryFiles() {}

// Resource path inside src/main/resources
public static final String ALIEN_CHALLENGE_RESOURCE = "alien_challenge.txt";
public static final String ALIEN_CHALLENGE_CODE = "alien_challenge";
}
107 changes: 107 additions & 0 deletions src/main/java/com/javarush/khmelov/config/StoryLoader.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package com.javarush.khmelov.config;

import com.javarush.khmelov.entity.Choice;
import com.javarush.khmelov.entity.EndingType;
import com.javarush.khmelov.entity.Story;
import com.javarush.khmelov.entity.StoryNode;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
import java.util.*;
Copy link
Owner

Choose a reason for hiding this comment

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

Использование 'import java.util.*;' считается плохой практикой (wildcard import). Следует импортировать только необходимые классы. [WARNING]


public class StoryLoader {

// Analyze story file
public Story load(String code, InputStream in) {
Copy link
Owner

Choose a reason for hiding this comment

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

Метод load слишком длинный и выполняет слишком много задач (парсинг, валидация, сборка). Рекомендуется разделить его на приватные методы согласно Single Responsibility Principle. [WARNING]

try (BufferedReader br = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8))) {
Copy link
Owner

Choose a reason for hiding this comment

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

Отсутствует логирование процесса загрузки. Вместо стандартных исключений стоит добавить логирование уровня INFO или DEBUG для отслеживания загрузки файлов. [INFO]


String title = "Untitled Story";
Copy link
Owner

Choose a reason for hiding this comment

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

Магическая строка 'Untitled Story' должна быть вынесена в константу. [INFO]

Map<String, MutableNode> temp = new LinkedHashMap<>();
String currentKey = null;

String raw;
int lineNo = 0;

while ((raw = br.readLine()) != null) {
lineNo++;
String line = raw.trim();
if (line.isEmpty()) continue;

if (line.startsWith("TITLE:")) {
Copy link
Owner

Choose a reason for hiding this comment

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

Использование множественных if-else для парсинга директив (TITLE, TEXT, CHOICE) выглядит громоздко. В Java 21 это можно элегантно заменить на switch со pattern matching. [WARNING]

title = line.substring("TITLE:".length()).trim();
continue;
}

if (line.startsWith("[") && line.endsWith("]") && line.length() > 2) {
currentKey = line.substring(1, line.length() - 1).trim();
temp.putIfAbsent(currentKey, new MutableNode(currentKey));
continue;
}

if (currentKey == null) {
throw new StoryParseException("Line outside node block at " + lineNo + ": " + raw);
Copy link
Owner

Choose a reason for hiding this comment

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

Текст ошибки 'Line outside node block' не локализован. Хотя сообщения в коде допустимы на английском, для единообразия проекта стоит придерживаться одного стиля. [INFO]

}

MutableNode node = temp.get(currentKey);

if (line.startsWith("TEXT:")) {
node.text = line.substring("TEXT:".length()).trim();
continue;
}

if (line.startsWith("CHOICE:")) {
String rest = line.substring("CHOICE:".length()).trim();
int arrow = rest.indexOf("->");
Copy link
Owner

Choose a reason for hiding this comment

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

Магический литерал '->' следует вынести в статическую константу класса. [INFO]

if (arrow < 0) throw new StoryParseException("Bad CHOICE at " + lineNo + ": " + raw);
String label = rest.substring(0, arrow).trim();
String next = rest.substring(arrow + 2).trim();
node.choices.add(new Choice(label, next));
continue;
}

if (line.startsWith("END:")) {
String val = line.substring("END:".length()).trim();
node.endingType = EndingType.fromRussian(val);
continue;
}

throw new StoryParseException("Unknown directive at " + lineNo + ": " + raw);
}

if (!temp.containsKey("START")) throw new StoryParseException("Missing [START] node.");

// build nodes
Map<String, StoryNode> nodes = new LinkedHashMap<>();
for (MutableNode mn : temp.values()) {
if (mn.text == null) throw new StoryParseException("Node [" + mn.key + "] missing TEXT:");
nodes.put(mn.key, new StoryNode(mn.key, mn.text, mn.choices, mn.endingType));
}

// validate links
for (StoryNode n : nodes.values()) {
for (Choice c : n.getChoices()) {
if (!nodes.containsKey(c.getNextNodeKey())) {
Copy link
Owner

Choose a reason for hiding this comment

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

Циклическая проверка ссылок выполняется вложенными циклами (O(n*m)). Для больших файлов это может быть медленно, хотя для квеста допустимо. [INFO]

throw new StoryParseException("Node [" + n.getKey() + "] points to missing node: " + c.getNextNodeKey());
}
}
}

return new Story(code, title, nodes);

} catch (IOException e) {
throw new StoryParseException("Failed to load story: " + code, e);
}
}

private static final class MutableNode {
final String key;
String text;
EndingType endingType;
final List<Choice> choices = new ArrayList<>();
Copy link
Owner

Choose a reason for hiding this comment

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

Поле choices в MutableNode не инкапсулировано. Несмотря на то, что класс приватный, лучше использовать getter/setter или методы доступа. [INFO]


MutableNode(String key) { this.key = key; }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.javarush.khmelov.config;

public class StoryParseException extends RuntimeException {
public StoryParseException(String message) { super(message); }
public StoryParseException(String message, Throwable cause) { super(message, cause); }
}
29 changes: 0 additions & 29 deletions src/main/java/com/javarush/khmelov/config/Winter.java

This file was deleted.

8 changes: 8 additions & 0 deletions src/main/java/com/javarush/khmelov/controller/Controller.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.javarush.khmelov.controller;

import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;

public interface Controller {
String handle(HttpServletRequest req, HttpServletResponse resp) throws Exception;
}
70 changes: 50 additions & 20 deletions src/main/java/com/javarush/khmelov/controller/FrontController.java
Original file line number Diff line number Diff line change
@@ -1,43 +1,73 @@
package com.javarush.khmelov.controller;

import com.javarush.khmelov.cmd.Command;
import com.javarush.khmelov.config.Winter;
import com.javarush.khmelov.entity.Role;
import jakarta.servlet.ServletConfig;
import com.javarush.khmelov.config.StoryFiles;
import com.javarush.khmelov.config.StoryLoader;
import com.javarush.khmelov.entity.Story;
import com.javarush.khmelov.repository.InMemoryStoryRepository;
import jakarta.servlet.ServletException;
import jakarta.servlet.annotation.WebServlet;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import java.io.InputStream;
import java.util.HashMap;
import java.util.Map;

import java.io.IOException;

Copy link
Owner

Choose a reason for hiding this comment

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

Карта routes не является потокобезопасной (HashMap), хотя инициализируется в init(). Для безопасности в Servlet среде лучше использовать Collections.unmodifiableMap или ConcurrentHashMap. [WARNING]

@WebServlet({"", "/home", "/list-user", "/edit-user"})
@WebServlet({"", "/home"})
public class FrontController extends HttpServlet {

private final HttpResolver httpResolver = Winter.find(HttpResolver.class);
private final Map<String, Controller> routes = new HashMap<>();

Copy link
Owner

Choose a reason for hiding this comment

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

Ресурс открывается через ClassLoader, но не проверяется на null перед использованием (до блока try). Хотя проверка есть внутри, структура кода может привести к NPE при чтении. [INFO]

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Command command = httpResolver.resolve(req);
String view = command.doGet(req);
String jsp = getJsp(view);
req.getRequestDispatcher(jsp).forward(req, resp);
public void init() throws ServletException {
InMemoryStoryRepository repo = new InMemoryStoryRepository();
StoryLoader loader = new StoryLoader();

try (InputStream in = Thread.currentThread()
.getContextClassLoader()
.getResourceAsStream(StoryFiles.ALIEN_CHALLENGE_RESOURCE)) {

Copy link
Owner

Choose a reason for hiding this comment

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

Использование e.getMessage() в ServletException — это хорошо, но теряется StackTrace в логах сервера. Рекомендуется использовать логгер (SLF4J). [WARNING]

if (in == null) {
throw new ServletException("Story resource not found on classpath: "
+ StoryFiles.ALIEN_CHALLENGE_RESOURCE
+ " (expected: src/main/resources/" + StoryFiles.ALIEN_CHALLENGE_RESOURCE + ")");
}

Story story = loader.load(StoryFiles.ALIEN_CHALLENGE_CODE, in);
repo.put(story);

} catch (Exception e) {
throw new ServletException("Failed to bootstrap stories", e);
}

StoryController storyController = new StoryController(repo);
routes.put("/", storyController);
routes.put("/home", storyController);
}

@Override
public void init(ServletConfig config) {
config.getServletContext().setAttribute("roles", Role.values());
Copy link
Owner

Choose a reason for hiding this comment

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

Логика выбора контроллера по умолчанию 'routes.get("/")' захардкожена. Стоит вынести '/' в константу. [INFO]

protected void doGet(HttpServletRequest req, HttpServletResponse resp) {
dispatch(req, resp);
}

private static String getJsp(String view) {
return "/WEB-INF/" + view + ".jsp";
@Override
Copy link
Owner

Choose a reason for hiding this comment

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

Пустой блок catch (ignored) — плохая практика. Даже если ошибка 'невозможна', ее стоит залогировать на уровне TRACE. [ERROR]

protected void doPost(HttpServletRequest req, HttpServletResponse resp) {
dispatch(req, resp);
}

@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Command command = httpResolver.resolve(req);
String redirect = command.doPost(req);
resp.sendRedirect(redirect);
private void dispatch(HttpServletRequest req, HttpServletResponse resp) {
try {
String path = req.getServletPath();
Controller controller = routes.getOrDefault(path, routes.get("/"));
String view = controller.handle(req, resp);
req.getRequestDispatcher(view).forward(req, resp);
} catch (Exception ex) {
req.setAttribute("errorMessage", ex.getMessage());
try {
req.getRequestDispatcher("/WEB-INF/error.jsp").forward(req, resp);
} catch (Exception ignored) {}
}
}
}
Loading