https://github.com/Spacier829/HangmanOOP
Ревью от Алексея @Raketa4000az
Игра в ООП стиле.
Недостатки реализации:
- Игра не запускается
C:\Users\alex1\.jdks\openjdk-22.0.1\bin\java.exe "-javaagent:C:\Program Files\JetBrains\IntelliJ IDEA Community Edition 2024.2.1\lib\idea_rt.jar=54609:C:\Program Files\JetBrains\IntelliJ IDEA Community Edition 2024.2.1\bin" -Dfile.encoding=UTF-8 -Dsun.stdout.encoding=UTF-8 -Dsun.stderr.encoding=UTF-8 -classpath C:\MyProg\JAVA\==OTHER\SERGEY_COURSE_REVIEW\hm-Spacier829\out\production\hm-Spacier829 Main
Exception in thread "main" java.lang.RuntimeException: java.io.FileNotFoundException: .\Dictionary\dictionary.txt (Системе не удается найти указанный путь)- Команды работают неправильно В игре используются только слова на русском языке
1. Старт
2. Выход
Введите '1' чтобы начать новую игру или '2', чтобы выйти:
100500
______
________
|/ !
|
|
|
|
|
|
--|---------- На старте не видно маски слова, игра не обьясняет юзеру, какие действия от него требуются. Только опытный висельник, вроде меня, сообразит, что делать дальше- ввести букву
______
________
|/ !
|
|
|
|
|
|
--|----------
Нет списка введенных букв
-
После отгадывания первой буквы, внезапно находится маска
п
п_____
________
|/ !
| (_)
|
|
|
|
|
|
--|----------
Можно ввести не одиночную букву, а челое слово за раз
-
приве_ + т = выиграл
приве_
________
|/ !
|
|
|
|
|
|
--|---------
т
Поздравляем! Вы выиграли!Хорошо:
- Есть ООП.
Замечания:
- Нейминг
-Не дублируй имя класса в названии метода класса
Game.gameLoop
//ПРАВИЛЬНО:
Game.loop
-Если в проекте есть класс MaskedWord то все переменные с именем, включающим это название, должны быть переменными этого класса
public class MaskedWord {
private String maskedWord;
//...
}
//ПРАВИЛЬНО:
public class MaskedWord {
private String маска;
//...
}
-UPPER_SNAKE только для констант, а это нет
private static String PATHNAME;
private static int WORDS_COUNT;-Сеттер должен что-то принимать во входящие аргументы и устанавливать в классе. Сеттер, который ничего не сетит- нонсенс. Сеттер, который что-то возвращает- нонсенс
int setWordsCount()
-Название должно обьяснять, что делает метод. "Проверить состояние игры"- ничего не объясняет. На самом деле этот метод проверяет, отгадано ли слово
/* class Game */
public boolean checkGameState() {
return maskedWord.getMaskedWord().equals(secretWord);
}
--
Oracle Java code conventions, part."Naming conventions"
Мартин, "Чистый код", гл.2
--- Цикл while всегда лучше читается, когда он написан в варианте while(){}, а не do{}while()- сразу видно условие входа в цикл. Любой do-while можно легко переделать на нормальный while
public Character letter() {
do {
//...
//...
//...
//...
//...
//...
//...
//...
//...
} while (true);
}
//ЛУЧШЕ:
public Character letter() {
while (true) {
//...
//...
//...
//...
//...
//...
//...
//...
//...
}
}- Если в блоке if есть return(break, continue), то else не пишется
if (letters.length == 0) {
System.out.println("Вы ничего не ввели, попробуйте еще раз.");
continue;
} else if (Character.isDigit(letters[0])) {
System.out.println("Вы ввели число, а не букву, попробуйте еще раз.");
continue;
}
//ПРАВИЛЬНО:
if (letters.length == 0) {
System.out.println("Вы ничего не ввели, попробуйте еще раз.");
continue;
}
if (Character.isDigit(letters[0])) {
System.out.println("Вы ввели число, а не букву, попробуйте еще раз.");
continue;
} - Нарушение DRY, магические буквы, числа, слова. Вводи константы
System.out.println("1. Старт");
System.out.println("2. Выход");
//...
System.out.println("Введите '1' чтобы начать новую игру или '2', чтобы выйти:");
switch (input[0]) {
case '1' -> //...
case '2' -> //...
}
//ПРАВИЛЬНО:
private final static String START = "1";
private final static String EXIT = "2";
System.out.printf("%s. Старт \n", START);
System.out.printf("%s. Выход \n", EXIT);
//...
System.out.printf("Введите '%s' чтобы начать новую игру или '%s', чтобы выйти:", ....);
switch (input[0]) {
case START -> //...
case EXIT -> //...
}
--
Фаулер, "Рефакторинг", гл.8 п."Замена магического числа символической константой"
--- class UserInput
-Нарушение SRP, при чем в очень затейлевой конфигурации. Здесь совмещены ответственности по вводу буквы от юзера, хранению введенных букв и действия с секретным словом и маской.
-При прочих равных, используй примитивные типы, а не обертки. Здесь Character не дает никаких дополнительных преимуществ перед char
public Character letter() {...}
public boolean identify(Character letter) {...}
//ПРАВИЛЬНО:
public char letter() {...}
public boolean identify(char letter) {...}
-Побочные эффекты, метод должен только идентифицировать букву(что бы это ни значило) и вернуть результат идентификации, но точно не должен при этом окрывать буквы в замаскированоом слове
public boolean identify(Character letter) {
//...
} else if (secretWord.contains(letter.toString())) {
maskedWord.openLetter(letter);
}
//...
return true;
}- class Menu
-Название вводит в заблуждение. От класса с названием "Меню" мы ожидаем другого. Это не меню, это запускатор игры.
- class MaskedWord, замаскированное слово
+Выявил базовую сущность и сделал ее классом, это хорошо. -Плохо то, что поведение сущности размазано по нескольким классам. Например, в Game находится метод, который определяет, что слово полностью открыто.
-Magic: -1, '_'-Метод void setMaskedWord(String maskedWord) неуместен и при использовании будет ломать логику работы класса. Например, будет у нас MaskedWord с секретным словом "Манчестер", а клиентский код через сеттер установит маску "Ливерпуль", то-то забавно будет. Спасает одно- сейчас метод не используется в программе. А значит он мусор и подлежит аннигиляции.
-Метод открытия буквы в стиле кота Шредингера- при вызове может открыть букву, если она есть и может не открыть букву, если ее нет.
О результатах действия клиентский код никогда не узнает
void openLetter(Character letter)
В случае, если мы пытаемся открыть букву, которой нет в слове, метод должен кидать исключение.
Альтернативный вариант- метод должен возвращать результат операции открыл/неоткрыл.
Это проще, но хуже- такой подход не обязывает клиентский код проверять результат, в отличии от проброса исключения, которое(исключение) бьет по лбу, если его не учитывать.
- class Dictionary
-Не делай init-методы, используй нормальный конструктор
public class Dictionary {
//...
public static void init(String pathName) {
//...
}
}
//ПРАВИЛЬНО:
public class Dictionary {
//...
public Dictionary(String pathName) {
//...
}
}Да, если для инициализации использовать конструктор, то не получится юзать статик-методы. Но это наоборот хорошо- "статические" классы(в смысле классы-хранилки static методов) в ООП нужно использовать редко и в специальных случаях, например в хелперах. А этот класс не должен содержать статических методов.
- enum GallowsRenderer, хартинки хангмана
-Рендерер должен осуществлять рендеринг, т.е. показывать изображение. А этот енам ничего не показывает, он просто содержит картинки хангмана. Класс нужно переназвать.
- class Game, основная игровая логика
-Нарушение SRP, DI. Класс не должен инициализировать сам себя, в данном случае- не должен сам создавать словарь, лезть в него и выуживать из него случайное слово для игры
public Game() {
//...
this.secretWord = Dictionary.getRandomWord();
this.maskedWord = new MaskedWord(secretWord);
}
//ПРАВИЛЬНО:
public Game(String secretWord) {
//...
this.maskedWord = new MaskedWord(secretWord);
}Гаме должен принимать в конструктор необходимые данные, в данном случае- String secretWord. В контексте ООП это имеет вполне прикладной смысл- текущая реализация Гаме жестко завязана на испольование Dictionary, который берет слова определенным способом- путем чтения файла. Если Гаме будет получать сикретВорд в конструктор, то на более высоком уровне, в данном случае в запускаторе, можно будет получать слово каким угодно образом. Например, из файла, по сети, силой мысли и т.д. и далее передавать его в конструктор Гаме. И тогда вне зависимости от способа получения слова, Гаме будет с ним работать одинаково.
-Длинные конструкции, которые поймут не только лишь все
System.out.println(GallowsRenderer.values()[errorsCount].getDrawing());
//ЛУЧШЕ:
GallowsRenderer[] картинкиХангмана = GallowsRenderer.values();
GallowsRenderer чистоКонкретноНужнаяКартинка = картинкиХангмана[errorsCount];
System.out.println(чистоКонкретноНужнаяКартинка.getDrawing());
//ЕЩЕ ЛУЧШЕ: вынести распечатку в отдельный метод printХангман(int номерКартинки)-Нарушение DRY. В классе Гаме имеется поле MaskedWord maskedWord, в состав которого входит String secretWord. Но такой же точно String secretWord, с тем же самым значением, является еще одним полем Гаме. И оба этих поля из Гаме используются в затейлевом инпутере UserInput userInput = new UserInput(maskedWord, secretWord, wrongInputLetters); // maskedWord.secretWord = "привет", secretWord = "привет" (прим. ред.)
-Вводи вспомогательные методы, делай программу более простой и понятной
if (errorsCount == ERROR_MAX_COUNT) {
System.out.println(GallowsRenderer.values()[errorsCount].getDrawing());
System.out.println("К сожалению Вы проиграли.");
System.out.println("Загаданное слово: " + secretWord);
} else if (checkGameState()) {
System.out.println("Поздравляем! Вы выиграли!");
}
//ПРАВИЛЬНО:
if(isПроиграл()) {
printПроиграл();
} else if (isВыиграл()) {
printВыиграл();
}- class Main, содержит точку входа main
+Только создает и запускает игру(в данном случае через запускатор), это хорошо.
-Сначала создает словарь, потом в запускаторе спрашивает, хотим ли мы играть. Если мы ответим "нет", то словарь создали зря. Словарь и Гаме нужно создавать только тогда, когда юзер отвечает "да".
Вывод: внять замечаниям.