HW_02

[Jura Khudyakov]

Есть небольшное Но, которое формально условию не соответствуют: .hpp вместо .h

Архитектурно решение, кажется, отличается от того, что предполагалось. Если представить граф, то Controller находится в корне, от него идут рёбра к Board и View, притом между Board и View связи кроме как через Controller нет. Он имеет на них указатели, но не владеет ими. У контроллера можно переключать и board, и view на ходу, при желании. Наверное, для симметрии можно сделать перключение у view контроллера на ходу.
Есть несколько неиспользуемых конкретно сейчас функций, но которые могут пригодится будущим View (например, getStepN)

Тестирование проводится местами не на поле 10*10 и 5 в ряд, а на меньших размерах, для удобства, благо 10 и 5 - не констаны и задаются при конструкции Board.

Возможно, многие решения покажутся странными, готов обсуждать.

[Sokolov Viacheslav]

O move: XXXX......
OOOOO.....
не хватает перевода строки

Какая мотивация enum class -ы явно наследовать от int?

Поскольку Player - enum class, удобнее просто X, O;
не уверен насчет NO_PLAYER, поскольку не во всех контекстах это валидное значение перечисления. Предлагаю подумать над использованием std::optional вместо этого.

Непонятно, зачем нужно состояние GAME_NOT_STARTED и метод initialize

33 static Player
34 convert(CellState? cell);

режет глаз

Классы стоит помечать как final, если от них не предполагается наследоваться, а конструкторы делать explicit (нужно ли пояснить, почему?)

Какая мотивация делать инициализацию полей в .h, а не в конструкторе?

Сейчас Board - пассивная сущность, в ней нет инвариантов. Но при этом зачем-то есть _currentPlayer, которым управляют все равно снаружи. Предлагаю либо убрать его из Board, либо добавить какие-то инварианты (например, что ходы чередуются).

66 Player horizontalCheck();
67 Player verticalCheck();
68 Player diagonal1Check();
69 Player diagonal2Check();

проще сделать один метод checkDirection и вызывать для какого-то из направлений {{1, 0}, {0, 1}, {1, 1}, {1, -1}}

8 enum class Mode {
9 STDIO_DEFAULT,

10 STDIO_SILENT,
11 NCURSES_DEFAULT
12 };

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

23 bool stopFlag = 0;

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

В NcursesBoardView?.hpp много того, что должно быть в .cpp, поскольку никому больше не нужно (инклуды, макросы, ...)

Лучше убрать зависимость BoardView? от GameController?. Сейчас сущности циклически зависят друг от друга, что плохо. BoardView? достаточно знать про Board.

Предполагалось, что у BoardView? ответственность - взаимодействие с пользователем. Методы stepBegin, stepEnd предполагают, что BoardView? более детально осведомлен о том, как устроена программа, что избыточно. Лучше убрать эти методы, по смыслу от BoardView? требуется только лишь отрисовать поле (в silent режиме при этом отрисовать только если игра окончена)

Лучше для тестов использовать не 1234, а говорящие имена (PlayerToCellX, ...), чтобы в случае не прохождения теста быстрее понимать, что именно не так

Сейчас между тестами дублируется код из-за ограничения "один тест - одна проверка", может быть, снять это ограничение?

В switch конструкциях стоит (при возможности) избегать default секции, потому что
при расширении перечисления компилятор не выдаст предупреждение, что новый элемент поддержан в switch, что потенциально может привести к ошибке.

211 assuming WIN_ROW_LENGTH > 0
если хочется написать подобное предположение, лучше поставить assert:

• assert будет проверен
• assert не устареет, в отличие от комментария (комментарии не компилируются)

75 return status() == Board::GameState::GAME_FINISHED_DRAW

76 77

status() == Board::GameState::GAME_FINISHED_O_WIN

status() == Board::GameState::GAME_FINISHED_X_WIN;

этот метод может располагаться рядом с Board::GameState?

82 return (0 <= pos.y && pos.y < static_cast<int>(_board->BOARD_HEIGHT)) && (0 <= pos.x && pos.x < static_cast<int>(_board->BOARD_WIDTH));

этот тоже

20 size_t width() const;
21 size_t height() const;
22 size_t winRowLength() const;
23 Board::BoardField? rawBoard() const;

непонятно, зачем эти методы в Controller

в main можно выделять на стэке, а не на куче; если произошло исключение, лучше вернуть ненулевой код возврата

ввод сейчас обрабатывается не так, как требуется по условию.
Примеры невалидных вводов:
1)
1
2
2)
1 2a
3)
0-0
от пользователя стоит ожидать невалидный ввод, поэтому нужно быть аккуратнее с исключениями

[Jura Khudyakov]

Исправил все замечания. Пара комментариев ниже.

"Классы стоит помечать как final, если от них не предполагается наследоваться, а конструкторы делать explicit (нужно ли пояснить, почему?)"
Добавил final-ов и explicit, где посчитал нужным. В остальных местах теоретически возможно наследование

Убрал currentPlayer, теперь он вычисляется на основе stepN, которым управляет уже Board
"Сейчас Board - пассивная сущность, в ней нет инвариантов. Но при этом зачем-то есть _currentPlayer, которым управляют все равно снаружи. Предлагаю либо убрать его из Board, либо добавить какие-то инварианты (например, что ходы чередуются)."

Она была нужна, так как компилятор иначе жаловался на control reached end of non-void function. Убрал default, теперь просто возвращаемое значение по умолчанию
"В switch конструкциях стоит (при возможности) избегать default секции, потому что
при расширении перечисления компилятор не выдаст предупреждение, что новый элемент поддержан в switch, что потенциально может привести к ошибке."

[Sokolov Viacheslav]

Ввод 08 008 считается некорректным, но он валиден

Если от класса предполагается наследование, стоит делать деструктор виртуальным.

24 if (!player.has_value()) {
25 assert(false);
26 }

assert(player)?

15 bool tryMakeMove(Board::Pos, std::optional<Board::Player> player);
16 bool tryMakeMove(Board::Pos);

выглядит странным снаружи отдавать такие команды GameController?, благо это его задача контроллировать ход игры

4 #include <ncurses.h>

не нужен в include/NcursesBoardView.hpp

15 virtual void terminate() = 0;
16 virtual void abort() = 0;

abort и terminate очень близкие по смыслу термины, лучше избегать синонимов на названиях

27 Mode _mode;

непонятно, зачем сохранять это поле в NcursesBoardView?

в конструкторе стоит проверить, что Mode передается разумный (а вообще он там не нужен, конечно)