https://github.com/ProgWrite/FirstProject
Ревью от Евгения @solid_jdk
Сканнер не закрыт. Есть мнение, что сканнер, который использует System.in можно не закрывать, и для простых и небольших программ это вполне так (JVM сама справится и все такое), но я все же придерживаюсь того, чтобы подобные моменты вошли в привычку. Главное помнить о том, чтобы не закрыть ресурс раньше времени. Какие есть варианты:
- Закрыть сканнер перед вызовом System.exit(0). Самый простой вариант, не требует переделывать структуру программы. Проблема: если программа завершится как-то иначе (например, будет выброшено исключение), то это возвращает нас к ситуации, когда управление ресурсом будет отдано на милость виртуальной машины. Соответственно, придется использовать try-catch, при этом сам close() тоже может выбросить исключение, что потребует дополнительных усилий, и в общем это приводит к следующему варианту.
- Использовать try-with-resources. Если не брать в расчет совсем уж радикальные ситуации, то это гарантия того, что сканнер будет закрыт. Но для этого придется переделывать структуру программы, чтобы try-with-resources не закрыл сканнер в неподходящий момент.
- Можно использовать shutdown hook (Runtime.getRuntime().addShutdownHook()), благодаря чему сканнер будет закрыт при завершении работы программы любыми способами, исключая катаклизмы (природные в том числе). Но с этой штукой нужно быть аккуратным.
метод main():
Хорошо, что он чистый настолько насколько это возможно, но есть нюанс, о котором дальше...
startMenu():
Казалось бы метод должен дать меню для старта, но в итоге он отвечает за запуск игры (и за ее остановку). Стартовое меню может быть частью запуска программы (как и прочие игровые итерации), но не наоборот. Поскольку вроде как для 1-го проекта акцент на ООП не делается (мб и на разных принципах тоже), но просто вот стоит сразу подсветить пару моментов. Т.е в данном случае страдает как нейминг метода, так и SRP из SOLID.
Второй пример нарушения SRP - это метод takeRandomWord(), который отвечает за то, чтобы сходить в файл и прочитать его, заполнить список полученными словами, сформировать рандом число, выбрать из списка элемент по индексу.
takeRandomWord():
- Каждый раз, когда игра начинается, то программа пойдет в файл и считает оттуда слова. Здесь подойдет какой-нибудь скромный кэш. Подумай, как это можно сделать.
- строки 38-39 - здесь кроется причина того, что не получается запустить игру не из среды разработки.
- e.printStackTrace(e) - это хорошо, но пользователю (игроку) не нужно видеть стак трейс ошибок, а таким образом он это увидит.
- строки 50-51 - создание отдельного объекта randomWord не имеет смысла. Лучше заинлайнить - return listWIthWords.get(randomIndex);. Это не просто может улучшить читаемость кода, но может и положительно повлиять на перформанс (мб компилятор сейчас это умеет делать и так, но мало ли).
gamePlay():
- строки 58-60 - данный код эквивалентен Arrays.fill() методу. Первым аргументом передаешь массив, который надо заполнить (randomWordChars), а вторым аргументом то, чем заполнить ("*").
- 66 строка - вместо списка лучше использовать HashSet. Метод contains() будет работать лучше - O(1) в среднем.
- 68 строка - лейбл здесь и соответственно в других местах необязательны. В данном случае, лейблы наверное даже ухудшают читаемость, особенно, когда назван INNER.
- 83 и 84 строки - дублирование checkRussianChar() метода. Причем без какой-либо на то причины в принципе.
- Основная логика - выглядит немного странно. 5.1. Переменная countStars: если на текущей итерации введенный символ в слове не найден, то countStarts инкрементируется. Если значение получилось равным длине загаданного слова, то инкрементируется уже переменная mistakes. Если и вводить доп переменную счетчик, то наверное для читаемости лучше пусть это будет подсчет правильно угаданных букв, и если за проход по слову их количество оказалось 0, то увеличиваем уже mistakes. Но все равно дополнительный счетчик кмк лучше заменить на булевый флаг isLetterFound:
boolean isLetterFound = false;
for (int i = 0; i < word.length; i++) {
if (word.charAt(i) == c) {
randomWordToChar[i] = c;
isLetterFound = true;
rightLetter++;
}
// короткое замыкание?
if (rightLetter == word.length) {
// победа
}
}
if (!letterFound) {
mistakes++;
}
// Или так: mistakes += isLetterFound ? 0 : 1;5.2. Общее. Желательно, чтобы названия переменных были сразу интуитивно понятны. rightLetter - правая буква? А как правая буква инкрементируется? Вариант для имени этой переменной - correctGuessCounter. То же самое и про переменную, которая считает звезды 😁
checkRussianChar():
- s1 не может быть isEmpty(), т.к пустой ввод до этого метода не дойдет - его обработает checkLength().
- s1 не может быть null, т.к Character.toString() можно считать как null safe метод.
- переменная boolean b не требуется.
- Первый вариант (этап) улучшения - "Убираем лишнее":
public static boolean checkRussianChar(char c) {
String s1 = Character.toString(c);
return s1.matches("^[а-яА-ЯёЁ]*$");
}- Другой момент - это s1.matches(). В таких небольших приложениях это будет незаметно, но подобный вариант плохо влияет на производительность, т.к каждый раз при вводе символа и выполнении этой проверке будет создаваться объект Pattern (каждый раз, т.к объект создается, выполняет свою работу и после им займется Garbage Collector), для работы которого также потребуется компилировать регулярное выражение. И так, как уже написал, для каждого ввода. Варианты: 5.1. Создать отдельный класс валидатор, в котором будет создан один объект Pattern (static final) и один раз будет вызван метод compile(), а в методе checkRussianChar(char c) будет такая логика: return PATTERN.matcher(Character.toString(c)).matches(); - где PATTERN это: private static final Pattern PATTERN = Pattern.compile(^[а-яА-ЯёЁ]*$"); 5.2. Альтернативой, которая не требует создавать отдельный класс и возиться с регуляркой в принципе, может быть Character.UnicodeScript.of(c) == Character.UnicodeScript.CYRILLIC; Судя по всему данный вариант может быть даже лучше, чем предыдущий.
Benchmark (testString) Mode Cnt Score Error Units
Separate Class a avgt 5 49,743 ± 1,874 ns/op
Separate Class b avgt 5 49,978 ± 2,653 ns/op
Separate Class c avgt 5 51,496 ± 4,159 ns/op
Separate Class д avgt 5 48,673 ± 1,680 ns/op
Original Method a avgt 5 186,789 ± 6,677 ns/op
Original Method b avgt 5 187,274 ± 19,176 ns/op
Original Method c avgt 5 184,884 ± 2,903 ns/op
Original Method д avgt 5 182,739 ± 1,708 ns/op
Character.UnicodeScript a avgt 5 12,144 ± 0,334 ns/op
Character.UnicodeScript b avgt 5 11,577 ± 0,315 ns/op
Character.UnicodeScript c avgt 5 11,530 ± 0,132 ns/op
Character.UnicodeScript д avgt 5 12,937 ± 0,245 ns/opРазное: название пакетов надо писать с маленькой буквы. Названия классов с большой, используя CamelCase формат. Ты вроде как это знаешь раз Hangman назван правильно, но вот непонятно, как при этом появился класс drawHangmanState.
PS. это не все моменты - только те, что мне показались наиболее интересными. PS2. Если форматирование "таблички" стало нечитабельным, то сорри - в любом случае основной момент по сути тут это цифры типа 48,673, 184,884, 11,530, которые указывают на затраченное в среднем время на выполнение операции (в наносекундах).