HW #2 (XO)

[Sokolov Viacheslav]

Доска выводится отраженной (см. тест из условия)

[Sokolov Viacheslav]

Вывод не соответствует условию. Где-то пробела не хватает, после Draw . должна быть.

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

BoardSigns?
Возможно, лучше назвать CellContent?

с текущим названием лучше не sign_X, sign_O, а просто X, O; перечисление можно именовать просто BoardSign? (будет логичнее смотреться в местах использования)

10 стоит вынести в constexpr константу

Про BoardSign?
В целом можно оставить как есть, но хочу акцентировать внимание вот на чем:
Используется в двух разных ипостасях (очередность игрока) и содержимое клетки. Между собой эти сущности связаны, но отличаются. Empty в качестве текущего игрока означает, что игра окончена и никто походить не может. Empty в качестве варианта хода не имеет смысла (и не должен быть допустим при вызове из интерфейса приложения).
То есть можно было бы разбить сущность на две:
CellContent?? (какой символ записан в клетке; X / O / Empty)
Player (игрок, XPlayer / OPlayer)
getPlayer бы возращал std::optional<Player>
canMove / move принимал бы Player
Отличие между типами могло бы уберечь от ошибок - на этапе компиляции могла бы быть проверка, что программист подумал о разнице между содержимым клетки и очередностью хода и имеет в виду именно очередность хода при вызове метода. Больше кода, зато меньше возможностей ошибиться и проще модифицировать (вдруг Вы захотите добавить препятствия на игровое поле?)

(Тем более в другом месте возникает Players все равно)

BoardView? лучше оставить без полей и конструктора, получится так называемый чистый интерфейс

14 void printBoard() const override ;

пробел лишний

лучше принимать не bool isSilent, а сделать
enum Mode{DEFAULT, SILENT, ...}, чтобы легко поддерживать новые режимы, а также избежать ошибок при смене значения флага.

В тестах тоже можно писать код :)
for (size_t i = 0; i < 10; ++i)

for (size_t j = 0; j < 10; ++j)

DO_CHECK(Board{}.canMove(i, j, BoardSigns::sign_O));

гораздо проще, понятнее, читаемее, изменяемее, чем текущий вариант

1023 Board *b = nullptr;
1024
1025 b = new Board();
1026 b->move(0, 0, BoardSigns::sign_O);
1027 DO_CHECK(b->canMove(0, 0, BoardSigns::sign_O) == false);
1028 DO_CHECK(b->canMove(0, 0, BoardSigns::sign_X) == false);
1029 delete b;
если я правильно понял, хочется очистить b. Для этого не обязательно на куче аллоцировать, есть несколько вариантов:
а) сделать отдельный scope {}
б) std::swap(b, Board{});

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

В testMove* стоит добавить перед каждым вызовом move проверку на возможность его выполнения - вдруг с точки зрения модели ход невалиден (а выполнение невалидного хода - UB, в частности, может быть, внутри класса уже подожжен фитиль, который взорвется, но не сразу)

Board::check*
можно объединить в один метод, который проверяет, есть ли 5 в ряд по направлению (направление же можно перебрать в цикле по {{0, 1}, {1, 0}, {1, 1}, {1, -1}}

Про int &x, int &y:
​​http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-out
лучше всего сделать
struct Position{int x; int y;};
использовать его везде, где используются x и y
и BoardView?? сделать метод (например)
virtual std::optional<Position> retrieveNextPosition() = 0;
optional или нет зависит от того, как хочется устроить обработку ситуации "разрыв соединения" / предварительное окончание игры / ...

if (x == -1 && y == -1) break;
так плохо, потому что происходит нетривиальное связывание каких-то значений с каким-то конкретным смыслом, что не закреплено явно через отдельный метод / именованную константу / ...

В endGame почему-то скопирован метод printBoard, хотя можно было не копировать, а просто его позвать (не надо так). Можно как-нибудь улучшить именование методов для контроллера и себя, чтобы printBoard все же только лишь печатал доску (независимо от режижма).

Сейчас ввод (stdio) работает не так, как требуется по условию.

[Sokolov Viacheslav]

Отсутствует проверка успешности ввода / вывода

[tarasov.denis]

Не получилось воспользоваться std::optional, т.к. мой компилятор почему-то его не видит

[Sokolov Viacheslav]

А какая версия компилятора и стандартной библиотеки?

[tarasov.denis]

Библиотека 17, g++ 7.4.0

[Sokolov Viacheslav]

Кажется, до g++ 8.2 c std::optional были какие-то проблемы. Возможно, стоит обновиться до 9.2
(может быть полезно ​https://launchpad.net/~jonathonf/+archive/ubuntu/gcc-9.2)

[Sokolov Viacheslav]

Не хватает пробела после * move:

  6 enum class BoardSign
  7 {
  8   Empty, X, O
  9 };
 10 
 11 enum class BoardPlayer
 12 {
 13   XPlayer, OPlayer
 14 };
 15 
 16 enum class BoardStates
 17 {
 18   draw, win_X, win_O, continues
 19 };
 20 

нет единого стиля именования элементов перечислений

45 bool canMove(const Point &coord, std::optional<BoardPlayer?> player) const;

здесь логично принимать просто BoardPlayer?

7 enum Players
8 {
9 player_X, player_O

10 };

непонятно, чем BoardPlayer? не подходит

14 std::optional<Point> inputCoord({-2, -2});

можно было оставить инициализированным std::nullopt по-умолчанию

непонятно, зачем одновременно и inputCoord, и moveCoord

в текущем варианте может оказаться, что здесь
32 _board.move(moveCoord, _board.getPlayer());
будет вызов с moveCoord = (-2, -2)

в целом весь метод написан небрежно

0 0a
считается валидным вводом, но таковым не является

не проверяется успешность вывода, с вводом тоже можно было бы работать аккуратнее

В testDraw10 можно было бы не копировать все подряд, а разделить данные для теста и вызовы методов

Непонятно, зачем нужен setPlayer - используется только в тестах, его использование потенциально ломает внутренние инварианты класса

x < 0

x >= BOARD_SIZE

y < 0

y >= BOARD_SIZE

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

114 if (player == BoardPlayer::OPlayer)
115 _player = BoardPlayer::XPlayer;
116 else if (player == BoardPlayer::XPlayer)
117 _player = BoardPlayer::OPlayer;
стоило бы вынести в отдельный метод