HW #2 (XO)

[Sokolov Viacheslav]

std::vector<std::vector<Player> >
удобно сделать using (Field?), чтобы не дублировать тип в разных местах

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

isCorrectCell - непонятное название, лучше именовать конкретнее, например, isInsideField

isEq5OnLine можно было бы назвать просто 5OnLine

Классы стоит помечать как final, если от них не предполагается наследоваться

Про 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 или нет зависит от того, как хочется устроить обработку ситуации "разрыв соединения" / предварительное окончание игры / ...

Предлагаю в BoardView? считать, что в наследнике доступен Board и убрать из интерфейса все, что может быть получено по Board (field, gameState, curPlayer, ...)

Что должно делать updateField не очень понятно, если речь про отображение, то можно использовать print / draw / redraw / ...

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

testGetCurPlayer1
Если curPlayer - это последний ходивший (а из названия это непонятно, что плохо), то на момент начала игры еще никто не ходил, тест некорректен

В тестах стоит проверить, что если игра закончилась, то нельзя сделать ход (canMove вернет false)

testGetField*
v можно сразу же инициализировать, есть необходимый конструктор

Сейчас в тестах не получится узнать, какой именно тест не пройден

3 std::ostream& operator<<(std::ostream &os, const GameState? &gameState) {
4 if (gameState == GameState::WIN_X) {
5 os << "X wins!";
6 }
7 else if (gameState == GameState::WIN_O) {
8 os << "O wins!";
9 }

10 else if (gameState == GameState::DRAW) {
11 os << "Draw.";
12 }
13 return os;
14 }

Лучше использовать switch, потому что тогда компилятор сможет помочь проверить, что все значения перечисления обработаны

29 Board::Board() {
30 field.resize(BOARDSIZE, std::vector<Player>(BOARDSIZE, Player::NONE));
31 }

можно инициализировать в списке инициализации

Стоит завести массив Directions, в котором перечислены возможные направления

field_has_none можно переименовать в game_in_progress / field_not_full, станет понятнее

15 if (x == -1 && y == -1) {
16 return;
17 }

так плохо, потому что происходит нетривиальное связывание каких-то значений с каким-то конкретным смыслом, что не закреплено явно через отдельный метод / именованную константу / ...

Кажется, если Controller владеет Board, то пусть и View тоже владеет

52 for (auto &line : field) {
53 for (auto &player : line) {

const auto&

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

Сейчас ввод обрабатывается не так, как того требует условие.

[Sokolov Viacheslav]

Поскольку Player - enum class, можно просто Player::X, Player::O

13 const Position forceEndGamePos = {-1, -1};

static, можно constexpr

In_progress
не camelCase, не snake_case, какой это стиль?

кажется, silent - это все же свойство stdioview, а не gamecontroller (поскольку в режиме curses все нужно отрисовывать)

6 assert(!std::cin.fail());

можно же исключениями, и надежнее и код проще

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

невалидный ввод все же нужно ожидать от пользователя

[Surkov Petr]

Исправления были закоммичены давно (ещё в срок), забыл обновить тикет

[Sokolov Viacheslav]

src/StdioBoardView.cpp: In member function ‘virtual void StdioBoardView::drawField(const std::vector<std::vector<CellContent> >&)’:
src/StdioBoardView.cpp:38:34: error: catching polymorphic type ‘class std::ios_base::failure’ by value [-Werror=catch-value=]
   38 |     catch (std::ostream::failure e) {
      |                                  ^
cc1plus: all warnings being treated as errors
Makefile:28: recipe for target 'obj/StdioBoardView.o' failed
make: *** [obj/StdioBoardView.o] Error 1

12 using std::vector;

в целом так лучше не далеть в заголовочном файле в global namespace

57 const Position Directions[4] = {{0, 1}, {1, 0}, {1, 1}, {-1, 1}};

static

15 in_progress,
16 Win_X,
17 Win_O,
18 Draw

все еще вопрос по стилю именования

5 #include <ncurses.h>

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

зачем-то есть пустой
src/BoardView.cpp

обработка ошибок от std::cout не единообразная, лучше всего было бы в main.cpp выставлять флаги по исключениям и там же эти исключения и обрабатывать

Ход "0 0a" обрабатывается как валидный