Крестики-Нолики. Оценка (критика) кода

Здравствуйте.

Изучаю C++ пару месяцев и захотелось для начала написать свою простую игру Крестики-Нолики (пока без GUI и ИИ).

Хотелось бы узнать разного рода мнения о моём коде, вообщем нужна ваша критика :)

обновленный линк на GitHub:
https://github.com/loktionov129/CPP_Tic-Tac-Toe

Класс Board отвечает за внутреннее представление игрового поля и игровую логику, а именно:
+ Геттер и Сеттер для конкретной ячейки поля (по координатам x и y)
+ Определение конца игры (ничья либо же победа одного из игроков)

Класс Game отвечает за визуальную часть:
+ Осуществление хода игроком
+ Отрисовка игровой информации и игрового поля на экран

Почему ДОСКА определяет завершена ли игра?
ДОСКА этого решать не должна.
Правила игры же определяются игрой, а не доской.
На одной и той же доске можно играть как в шахматы, так и в шашки.

Game *game = new Game();
game->Start();
delete game;

И зачем здесь динамическая память?
Просто «потому что могу»?

void ClearBoard() const;

Зачем оно const, если здесь явно видно,
что оно не должно быть таковым?
Очищение доски означает изменение данных.
Из-за таких вот манипуляций,
пришлось сделать board mutable.
Тоже самое с

bool Board::SetCell(int x, int y, char player) const

В общем, работайте. :)

В дополнение к предыдущему оратору:

0). Плохо организован репозиторий на GitHub. Вместо того, что бы скачать 8.39кБ исходников крестиков-ноликов, пришлось качать все проекты (26МБ) целиком. Что не есть гут ((

1). setlocale нормально работает, если исходники сохранены в кодировке windows-1251. У тебя файлы с кириллицей сохранены в UTF-8. В итоге в консоли нечитаемый текст.

2). Юзабилити: при игре неудобно «вычислять» координаты следующего хода. Тем более с нумерацией от 0.

3). Ничейную позицию можно определить гораздо раньше, чем вся доска будет заполнена.

4). Алгоритм проверки победителя учитывает только главные диагонали поля. Например при игре на поле 5х5 с условием «3 в ряд» позиция

O X - - -
- O X - -
- - - X -
- - - - -
- - - - -

[Игрок 2] Ваш ход (x; y):

не считается выигрышем.

5). В классах и методах каша. Надо чётко определить за какие данные и за какие действия отвечает класс Board, а за какие Game. Логику игры надо перенести в Game, а отображение доски — в Board. Всё ООП разбивается об эту ошибку и превращается в спагетти. Кроме того, при нормальном проектировании классов некоторые методы (из существующих) станут гораздо проще. (Например отпадёт необходимость передавать this как параметр.)

6). Каша из char player и enum { STATE_NEXT_STEP = -1, STATE_DRAW = 0, STATE_X_WINNER, STATE_O_WINNER}. (Кстати, enum { ... } имеет тип int.)

7). #pragma once и #ifndef BLA_BLA_H ... #endif — масло масляное. Однако первое «масло» зависит от компилятора. В данном случае это ни на что не влияет, но для чистоты кода #pragma once лучше убрать.

Благодарю за оперативность :)

В классах и методах каша. Надо чётко определить за какие данные и за
какие действия отвечает класс Board, а за какие Game. Логику игры
надо перенести в Game, а отображение доски — в Board. Всё ООП
разбивается об эту ошибку и превращается в спагетти.

Почему ДОСКА определяет завершена ли игра? ДОСКА этого решать не
должна. Правила игры же определяются игрой, а не доской.

Здесь я отталкивался от 'Разделения мух и котлет'. Т.е. Board — Model, Game — View.

На одной и той же доске можно играть как в шахматы, так и в шашки.

Если так рассуждать, то да, логично будет перенести определение конца игры в Game из Board.
На данный момент, как я думаю, так у меня самая важная проблема — это архитектура. Можете что-нибудь посоветовать насчёт проектирования ?

И зачем здесь динамическая память? Просто «потому что могу»?

    Game *game = new Game();
>      game->Start();
>      delete game;

Где-то читал что лучше память под объекты выделять в куче, а под стандартные типы данных — на стеке.

В общем, работайте. :)

Хорошо, спасибо. Насчёт const тоже читал что его лучше вставлять везде, где только можно

Юзабилити: при игре неудобно «вычислять» координаты следующего хода.

первым числом вводится строка (y), вторым — столбец (x).

Тем более с нумерацией от 0.

Можно прибавить единичку после ввода с клавиатуры, а вообще позже хочу прикрутить GUI.

Алгоритм проверки победителя учитывает только главные диагонали поля.
Например при игре на поле 5х5 с условием «3 в ряд»

Здесь недоглядел, спасибо.

Кроме того, при нормальном проектировании классов некоторые методы
(из существующих) станут гораздо проще. (Например отпадёт
необходимость передавать this как параметр.)

Можно пример кода? Изначально у меня было несколько однотипных функций для определения победителя, затем их объединил в одну и вызывал её с параметром-указателем на функцию.

Каша из char player и enum { STATE_NEXT_STEP = -1, STATE_DRAW = 0,
STATE_X_WINNER, STATE_O_WINNER}. (Кстати, enum { ... } имеет тип int.)

т.е. char заменить на int, я правильно понял?

Т.е. Board — Model, Game — View.

Тогда где Controller?

Можете что-нибудь посоветовать насчёт проектирования ?

Ну вот чертить диаграммы здесь точно неудобно.
Можете сделать, например, классы Board, Game, Player.
Board — знает только о себе,
т.е. что, где у нее находится.
По сути — матрица.

User — знает как сделать ход (например, подождать ввода пользователя, ну или посчитать ходи, если это игрок с ИИ). Скорее всего класс абстрактный.

Game знает правила игры (или нет?),
знает как опрашивать доску,
и знает какой юзвер сейчас должен ходить.

Где-то читал что лучше память под объекты выделять в куче, а под стандартные типы данных — на стеке.

Game не настолько большой объект, чтобы переживать за него.
Зато если вылетит исключение из game->Start(), то,
Вам сейчас нужно позаботиться о обработке исключения.

Насчёт const тоже читал что его лучше вставлять везде, где только можно

Там где это разумно и не мешает — да.
А если пихать его куда попало,
можно даже компилятору оптимизации поломать,
не говоря уже о Вашем случае,
оно там ну вот совсем не к месту.

Правило такое — если функция-член не изменяет объект,
то делаем её const, если она не выбрасывает исключений, то noexcept.

Накатал: https://github.com/croessmah/console_xo

Сделано под 4 игрока (символов больше не придумал). :D
Если надо под два, то из вектора users удаляем двоих.
Всё, больше ничего менять не надо.

Морочить голову выносом игрового цикла за main даже не стал.

Можете сделать, например, классы Board, Game, Player.

Обязательно попробую переработать архитектуру, спасибо.

Game не настолько большой объект, чтобы переживать за него. Зато если
вылетит исключение из game->Start(), то, Вам сейчас нужно
позаботиться о обработке исключения.
То есть то высказывание всё-таки было верно? Тогда если обычный указатель небезопасен в таком случае, может тогда лучше использовать smart poiner, напр std::unique_ptr/std::shared_ptr?

Правило такое — если функция-член не изменяет объект, то делаем её
const, если она не выбрасывает исключений, то noexcept.

Хорошо, теперь более-менее разобрался с const.

Накатал: https://github.com/croessmah/console_xo

Я рад, что смог заинтересовать Вас, и Вы нашли время 'накатать' по-своему.
Будет чем заняться ближайшее время, спасибо еще раз.

С наступающим :)

и Вы нашли время 'накатать' по-своему.

Ну, не совсем по-моему.
Просто я лентяй, а задумка не под консоль изначальна.
Посему, всё что связано с консолью наляпано как спагетти. )))

С наступающим :)

И Вас! Всего наилучшего. :)

Кроме того, при нормальном проектировании классов некоторые методы (из существующих) станут гораздо проще. (Например отпадёт необходимость передавать this как параметр.)

Можно пример кода?

Мне писать — не имеет смысла. Когда приведёшь в порядок классы, оно само у тебя сложится.

Юзабилити: при игре неудобно «вычислять» координаты следующего хода.

первым числом вводится строка (y), вторым — столбец (x).

Это всё понятно. Просто неудобно.
Надо либо клавишами управления курсором выбирать позицию, либо мышью.
Кстати, первым вводится таки x (номер столбца). По крайней мере, в первой версии.

Т.е. Board — Model, Game — View.

Тогда где Controller?

Тут есть ещё один момент. Думаем в сторону «переходим от консоли к GUI». Что нужно изменить в программе, что бы это сделать? И как нужно изменить программу, что бы такой переход потребовал замены (переписывания) только одного модуля?

Тут есть ещё один момент. Думаем в сторону «переходим от консоли к
GUI». Что нужно изменить в программе, что бы это сделать? И как нужно
изменить программу, что бы такой переход потребовал замены
(переписывания) только одного модуля?

Такая мысль и была изначально.
Поэтому, чтобы при переходе было минимум модификаций кода логика сейчас такая, какая она есть.

Класс Board(Model) не изменяется, только Game (View). Итак, план действий:
0) Задание настроек игры

Game::Game()

 std::cout << "Размер поля: ";
 std::cin >> field_size;

1) Очищение консоли cls||clear, и вывод игрового поля

void Game::Display()
        switch (board->GetCell(x, y))
        {
        case Board::CELL_X: std::cout << "X "; break;
        case Board::CELL_O: std::cout << "O "; break;
        default: std::cout << "- "; break;
        }

2) Осуществление хода

void Game::Move()
 std::cin >> y >> x;

3) Определение состояния игры: ничья/победа/следующий ход

void Game::Update()
 std::cout << message << " Желаете сыграть еще раз? 1/0: ";

Класс Board(Model)

Доска — это не вся модель игры.
У игры же есть состояние, игроки и т.д.

loktionov129, понятия «модель», «представление» не надо понимать так буквально. Croessmah дал совершенно правильные рекомендации по структуре. Я просто добавил к его схеме ещё один элемент.

Ввод-вывод, UI — обычно системозависимы. Любой шаг в сторону от стандартных <iostream> и <cstdio> тянет за собой системозависимые вызовы. Не говоря уж о переходе от консольного приложения к приложению с GUI. Поэтому лучше написать отдельный абстрактный класс, который будет удовлетворять все нужды игры по вводу-выводу. И от него наследовать класс, реализующий методы для работы с консолью.

Когда понадобится GUI, придётся написать только класс-потомок для работы с GUI, не затрагивая другие части программы. Да, конечно изменится и функция main. Поэтому она не должна содержать функциональности самой игры, а служить только для её запуска.

Aлгоритм проверки победителя учитывает только главные диагонали поля.
Например при игре на поле 5х5 с условием «3 в ряд» позиция не
считается выигрышем.

O X - - -
- O X - -
- - - X -
- - - - -
- - - - -

Исправил.

Ввод-вывод, UI — обычно системозависимы.

Сделал на Qt.

https://github.com/loktionov129/CPP_Tic-Tac-Toe

Кроме того, при нормальном проектировании классов некоторые методы (из
существующих) станут гораздо проще. (Например отпадёт необходимость
передавать this как параметр.)

Тоже исправил.

3). Ничейную позицию можно определить гораздо раньше, чем вся доска
будет заполнена.

А вот здесь не понял. Как это сделать проще ?

А вот здесь не понял.

Если больше нет возможности составить линию указанной длины.

Например, при поле 5x5 и необходимости собрать линию из 4:

|x|0|x|x| |
| |x|0|x|x|
|0| |x|0|0|
|x|0|0|0|x|
| | |0|x| |

как видите, больше ни у кого нет возможности построить линию длиной в 4, т.е. дальнейшая игра — пустая трата времени.

Как это сделать проще ?

Опять же, зависит от того, как это у Вас всё устроено.
В принципе, можно из каждой свободной клетки «пускать луч» в каждом направлении и смотреть, можно ли там составить линию заданной длины.
Если нельзя, тогда ничья. Но с самого начала выполнять эту операцию бессмысленно.

как видите, больше ни у кого нет возможности построить линию длиной в
4, т.е. дальнейшая игра — пустая трата времени.

Окей, спасибо. Работаем дальше.. :)

Пока что на этот случай есть void RestartGame(char *message = 0) override;

Как жаль что нету исходников, может еще есть где-то ? Спасибо за ответ.