0

Изучаю Java 3-4 месяца, дошел до многопоточности. Решил написать свою простую программу из того, что знаю.
Просьба - оцените код, критика и камни в огород приветствуются, но исключительно по делу.

XO Game

Моё мнение - код можно упростить(слишком громоздкий), но не знаю как..

3
  • 1
    Думал, сейчас многопоточность погляжу, а вы, видимо просто о ней написали. Бегло просмотрел - только 2 незначительных замечания бросились в глаза: 1. Computer.step() - выход из цикла зависит от рандома и чем больше полей занято, тем дольше ждать. Терпимо, наверное для игры 3х3. Лучше выбирать из заведомо свободных ячеек. 2. Field.showField() - условие if (i == 3 | i == 6) прокатит, конечно, но это битовая операция, лучше пользоваться логической ||.
    – Vladimir
    23 сен 2019 в 8:43
  • первое , что мне бросается в глаза - статические методы, а также явное нарушение паттерна grasp information expert. вопрос в том, что определять победу должно поле, а не игрок. как и иметь методы, которые позволят ходить игрокам - отмечать ходы на поле. в этом случае все, кроме метода, который отвечает за ход и метода, который проверяет победную комбинацию, будет иметь приватный доступ и никаких статических полей не потребуется. способ определения победы, на мой взгляд, стоит пересмотреть. делать цикл с свичом , который конкотинирует строки, чтобы потом их сравнить?? подумайте над этим 23 сен 2019 в 9:17
  • Весь код должен быть прямо в вопросе. Если он слишком обширен - сократите до отдельного куска и задавайте "вопрос" по нему
    – Kromster
    23 сен 2019 в 10:14

1 ответ 1

7

1) Бесполезные поля типа private boolean oneMoreGame = true;. Добавляя флаг в класс, вы добавляете

  • сложность в состояние класса. Например, с одним bool флагом у клсса будет 2 состояния, с 2 флагами - 4 состояния. с 10 флагами - 1024 состояния.
  • больше сложность класса - труднее его тестировать и поддерживать

Вы легко можете вернуть bool из метода restart() и избавиться от флага.

2) class Game, void go(...), private void introduction() - почему для каких то классов/методов вы указываете уровень доступа, а для каких то нет? Будьте консистенты в этом, указывайте уровень доступа явно, это хорошая практика.

3) Смешали в кучу логику для работы с консолью и логику игры. Это, для вашей маленькой игры, не критично, но вообще, если вы хотите сделать всё правильно, то надо разделять ответсвенности классов. Это буква S и SOLID принципов

4) Почему в момент, когда вы считываете ответ от юзера answer = sc.nextByte(); если юзер ввел не то, что программа ожидает, вы возвращаетесь из метода? Почему бы не переспрашивать вопрос там, где вы его задаёте?

5) Не используйте статические поля static char[] cellValue = new char[9]; без крайней необходимости. Создавая статическое поле, вы стреляете себе в ногу, вы ограничиваете сами себя тем, что все экземпляры Field будут вынуждены шарить одно и то же состояние. Запомните - у вас должна быть веская причина испольщовать статику. В данном случае она не нужна, просто создайте одно поле и работает с ним. Если когда-либо захотите создать мультипеер на 100 полей, статика вам будет только мешать.

6) if (answer < 1 || answer > 2) { бесполезная конструкция, используйте default для switch который у вас там же находится.

7) Ваш класс Computer жестко связан со статическими членами Field. Почему бы вместо этого не передавать в Field в метод void step() как параметр? Тогда ваш Computer сможет работать с любым Field и его подклассами. Это буква L из SOLID. Мало того, ваш компьютер ещё и сам пишет в консоль, что опять нарушает S из SOLID. То же самое для Player

8) Магические числа лучше выносить в константы i < 9, sumOfCellStatus() == 9, if (i == 3 | i == 6)

9) Никогда не используйте битовые операции в условии if (i == 3 | i == 6), они убивают оптимизации компилятора при расчете выражения и не предназначены для подобной конструкции.

10) Почему то ваша игра прнимает игроков как параметры, но сама создает поле. Почему бы не принимать поле тоже? Это будет сразу O и D из SOLID.

11) Вы выставляете некоторые поля вашего класса напоказ всему пакету (static byte[] cellStatus = new byte[9];), то есть все клиенты вашего класса узнают о внутреннем остоянии поля, а само поле больше не контроллирует доступ ко внутреннему состоянию. То есть достаточно кому то из клиентов получить ссылку на эти массивы и он может делать с полем всё, что захочет, и класс поля никак от этого не защитится. Чтобы такого избежать, скрывайте детали реализации ваших классов, не давайте доступ к внутреннему состоянию кому попало и без причины. Почитайте про инкапсуляцию.

12) Логика проверки, закончилась игра или нет, относится к игре, а не к игроку. Снова S из SOLID

По java фишкам я вам ничего не могу подсказать, я не пишу на java, но могу позказать пару примеров на C#. Вот, например, небольшая игра с графикой, или вот меню с состояниями в консоли. Те примеры кода тоже не идеальны, но я тогда просто отвечал на вопросы, а не стремился написать образцовый код.

Из того, чего вам не хватает:

1) Больше практики с ООП

2) Больше знаний по качеству написания кода, приницпам SOLID, паттернам (начните с любой книги по патернам)

3) Стоит иметь какой то набор рекомендаций, как писать код. У джавистов есть такой - книжка Джошуа Блоха - effective java.

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими пользовательским соглашением, политикой конфиденциальности и политикой о куки

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.