Asenim/TurtleOnaRock - Gallow.md

## TurtleOnaRock - Gallow.md

      
    Raw
  

              TurtleOnaRock - Gallow.md
            
          
    https://github.com/TurtleOnaRock/Gallow
Ревью от Алексея @Raketa4000az

[Alexander]
Игра в ООП стиле. После первого рефакторинга.
Недостатки реализации:

Команды не работают

Хотите сыграть в "Висилицу"? [д]а/[н]ет :Д
Хотите сыграть в "Висилицу"? [д]а/[н]ет :Н
Хотите сыграть в "Висилицу"? [д]а/[н]ет :


Нет списка неправильно введенных букв.


Ненужные неудобства для пользователя.
Если программе нужны буквы определенного регистра, то она сама должна переводить буквы в этот регистр, а не создавать юзеру неудобства
Ваша буква: Ф
В игре используються только русские строчные буквы.
Ваша буква:


Хорошо:

Простой понятный алгоритм

Замечания:

Нейминг
-UPPER_SNAKE только для констант, а это нет

private final int DEFAULT_VALUE_STAGE = 0;

//ПРАВИЛЬНО ТАК:
private static final int DEFAULT_VALUE_STAGE = 0;

//ИЛИ ТАК:
private final int defaultValueStage = 0;
-КОНСТАНТЫ_ПИШУТСЯ_СТИЛЕМ_UPPER_SNAKE
private static final String[] frames = ...

//ПРАВИЛЬНО:
private static final String[] FRAMES = ...
-Название вводит в заблуждение, это не поиск символа, это ввод символа
class CharFinder

//ПРАВИЛЬНО:
class CharInput
-Название путает на ровном месте
private Scanner in;
//...
this.in = new Scanner(System.in);

//ПРАВИЛЬНО:
private Scanner scanner;
//...
this.scanner = new Scanner(System.in);
--
Oracle Java code conventions, part."Naming conventions"
Мартин, "Чистый код", гл.2
-- 

Нарушение DRY, магические буквы, числа, слова. Вводи константы

{...} while (answer != 'д' && answer != 'н');
return (answer == 'д');

//ПРАВИЛЬНО:
private final static char YES = 'д';
private final static char NO = 'н';

{...} while (answer != YES && answer != NO);
return answer == YES;

Нарушение конвенции кода

for (int i = 0; i < secret.length; i++)
  this.secret[i] = Constants.MASK_SIGN;

//ПРАВИЛЬНО:
for (int i = 0; i < secret.length; i++) {
  this.secret[i] = Constants.MASK_SIGN;
}

Избыточно

if (this.word.charAt(i) == c) {
  result = true;  
  return result;
}

//ПРАВИЛЬНО:
if (this.word.charAt(i) == c) {
  return true;
}

Создавай вспомогательные методы, делай программу более простой и понятной

while (puzzle.isSecret() && man.isAlive()) {...}

//ПРАВИЛЬНО:
while (isИдетИгра()) {...}

private boolean isИдетИгра()() {
  return !isПроиграл() && !isВыиграл();
}

private boolean isПроиграл()() {
  return !man.isAlive();
}

private boolean isВыиграл()() {
  return !puzzle.isSecret();
}

Если в блоке if есть return(break, continue, throw, exit и т.д.), то else не пишется - это не имеет смысла

if (line.length() == 1) {
  return line.charAt(0);
} else {
  System.out.print(Constants.ONE_LETTER_REQUEST);
}

//ПРАВИЛЬНО:
if (line.length() == 1) {
  return line.charAt(0);
}
System.out.print(Constants.ONE_LETTER_REQUEST);

class Puzzle, реализация сущности "замаскированное слово"
+В целом норм

-Если в слове отсутствует буква, которую хотим открыть, то метод должен бросать исключение- открывать можно только те буквы, которые есть в слове
C
void update(char c)
-Магическое: '_'

class HangedMan, модель висельник
+В целом норм

-limit тоже нужно показывать через геттер. Чтобы, например, можно было подсчитать количество оставшихся попыток

class Constants, константы для всех классов
-Нарушение low coupling, свалка констант- содержит константы всех классов, т.о. все классы имеют зависимость на этот класс.
Класс аннигилировать, константы перенести в те классы, которые их используют.

Например, ONE_LETTER_REQUEST перенести в class CharFinder.
Сейчас class CharFinder не может существовать без class Constants. И если например будешь делать игру "Поле чудес", то не сможешь взять туда CharFinder для ввода букв.
Потому что без Constants он не работает, а в другой программе будут актуальны другие константы.

https://www.baeldung.com/java-constants-good-practices


class Dictionary
+Все ок


class CharFinder, ввод символа пользователем
+Все ок, кроме названия


class GallowRender, распечатка картинок висельника
-Нарушение DRY, дублирует значение лимита висельника


/* class HangedMan */
private final int DEFAULT_VALUE_LIMIT = 6;

/* class GallowRender */
private static final String LIMIT_MSG = "/6\n";
-Этот класс не должен знать лимит висельника потому что он не должен распечатывать информацию про лимит.
Этот класс должен распечатывать только картинки. Сопроводительный текст должен печатать кто-то другой, в данном случае- Game
System.out.print(MISTAKE_MSG + stage + LIMIT_MSG);  //в GallowRender не надо это делать
Либо, в крайнем случае, параметр limit метод должен принимать в аргументы
public class GallowRender {
  //...
  public static void showMistakes(int stage, int limit) {
    System.out.print(MISTAKE_MSG + stage + "/" + limit);
  }
}
-Если класс печатает картинки не только по номеру, но и по состоянию игры(выиграл, проиграл), то эти картинки нужно хранить в трех разных переменных
  public static void showGallow(int stage) {
    System.out.println(frames[stage]);
  }

  public static void showWin() {
    System.out.println(frames[Constants.WIN_STAGE]);
  }

  public static void showLose() {
    System.out.println(frames[Constants.LOSE_STAGE]);
  }

//ПРАВИЛЬНО:
  public static void showGallow(int stage) {
    System.out.println(FRAMES[stage]);
  }

  public static void showWin() {
    System.out.println(PICTURE_WIN_STAGE);
  }

  public static void showLose() {
    System.out.println(PICTURE_LOSE_STAGE);
  }

class Launcher, содержит точку входа main
-Никогда не возвращай null

private static Dictionary getDictionary(String pathToFile) {
  //...
  System.out.println(Constants.ERROR_FILE);
  return null;
  //...
}
Возврат null повышает риск возникновения NullPointerException в программе
--
Мартин, "Чистый код", гл.7.7-8
Ютуб, Немчинский "Почему нельзя возвращать NULL?"
--
-Проверяй команды независимо от регистра
{...} while (answer != 'д' && answer != 'н');

//ПРАВИЛЬНО:
private final static char YES = 'д';
//...
{...} while (Character.toLowerCase(answer) != YES ....);

//ЕЩЕ ЛУЧШЕ:
{...} while (!isCommand(YES, answer) && !isCommand(NO, answer));

private static boolean isCommand(char command, char symbol) {
  return Character.toLowerCase(symbol) == command;
}

//ЕЩЕ ЕЩЕ ЛУЧШЕ:
{...} while (!isCommand(answer));

private static boolean isCommand(char symbol) {
  return isCommand(YES, symbol) || isCommand(NO, symbol);
}

private static boolean isCommand(char command, char symbol) {
  return Character.toLowerCase(symbol) == command;
}
-Если кастомный класс может вернуть null, то нужно всегда проверять возвращаемое значение на null. Но однажды это можно забыть сделать и это вызовет NPE, см. "Почему нельзя возвращать NULL?"
Поэтому кастомные классы не должны возвращть null. Здесь getDictionary(...) в случае невозможности создать словарь должен либо возвращать Optional, либо бросать исключение
dictionary = getDictionary(Constants.PATH_TO_DICTIONARY);
if (dictionary == null) {...}

class Game
+Хорошо, что принимает в конструктор текст слова, а не грузит его сам из Словаря- соблюдение DI

public Game (String word){...}
-Отрицательные условия воспринимаются хуже положительных
private static boolean isNotRussian(char letter) {
  return !(((letter >= 'а') && (letter <= 'я')) || letter == 'ё');
}

//ПРАВИЛЬНО:
private static boolean isRussian(char letter) {
  letter = Character.toLowerCase(letter);  //большие русские буквы- они тоже русские
  return (letter >= 'а' && letter <= 'я') || letter == 'ё';
}
Вывод: стало значительно лучше. Можно пробовать Симуляцию.
#ревью #виселица