HW #2

[Solovyev Gleb]

Очень нужна полная проверка с комментариями по стилю и по логике, если возможно.

Много мест, где пришлось принимать неоднозначные решения.
Где-то остались вопросы.

[Sokolov Viacheslav]

Вывод не соответствует условию.

Makefile:
make clean не отработает, если нет файлов hw_02 / test_hw_02

В hw_02 / test_hw_02 зависимости же тоже можно через $(OOO) указывать?

GameController? : инклуды не нужны, достаточно forward-declarations

Какая мотивация помещать TicTacToeGame? как поле в GameView??

В Move смешаны две разных независимых вещи: собственно ход (Position) и информация об успешности взаимодействия с пользователем (MoveType?). Стоит либо именовать более подходящим образом, чтобы из названия было понятно, что это за информация (потому что речь на самом деле не о ходе, а о взаимодействии с пользователем), либо разнести. Например, использовать std::optional, чтобы явно выразить, что хода может и не быть.

virtual void getMove(Move &mv) : ​http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-out

У структур можно не делать конструктор

Непонятно, зачем плодить сущности и заводить height/width, если есть Position (в текущем варианте одно и то же - координаты поля - задается двумя разными способами)

Какая мотивация использовать c-style массивы, а не std::array/std::vector?

Какая мотивация дружить?
friend class TicTacToeGameTest?;

Вместо пары макросов XO_TEST / PASSED лучше завести специальный класс XOTestCase, который в конструкторе произведет регистрацию, а деструкторе зафиксирует статус теста (passed / failed).

for (size_t h = cellFrom._y; h < cellTo._y + 1; h++) {

for (size_t w = cellFrom._x; w < cellTo._x + 1; w++) {

b[Position(h, w)] = bState;

}

}

<=?

DO_CHECK(&g._board == &g.getBoard())
DO_CHECK(g._gameState == g.getState())
DO_CHECK(g._curTurn == g.getCurTurn())

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

если какая-то проверка не выполнится, соответствующий тест все равно будет passed, что странно

лучше тестировать какой-нибудь логический блок, например, "сколько символов в ряд", чем кусочки реализации. Это позволит не переделывать тесты, если реализация поменяется (а пользователям класса в общем-то все равно, как именно реализовано внутри, лишь бы работало)

g._leftEmpty = 0;

тест сломал внутренние инварианты класса. Лучше, чтобы тест не знал о внутренней реализации, а смотрел только на публичные методы и проверял, что внешнее поведение соответствует ожидаемому. То есть у класса ответственность - соблюдение инвариантов при вызове публичных методов, у тесты ответственность - проверка выполнения инвариантов. Лезть внутрь класса и ломать инвариант, а потом смотреть, как ведет себя класс со сломанным инвариантом - непонятно, зачем, потому что "in the wild" класс со сломанным инвариантом вообще не должен возникать.

лучше назвать не move123, а game123

можно упростить именование
printGameIsFinished -> printGameFinished

можно подумать о смене именования:
BoardState? --> CellContent??

проверку "5 в ряд" лучше декомпозировать. Направление можно перебрать в цикле.

case GameState::GOING: this case must be unreachable

std::cerr << "GameController? error, game is not finished." << std::endl;
break;

}

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

Ввод 0-0 интерпретируется не так, как того требует условие.

[Sokolov Viacheslav]

12 bool StdioGameView::checkInputSyntaxFormat(std::string &input_line) const {
const&

37 } else if (y < 0

x < 0) {

38 userAction._actType = ActionType::BAD_INPUT_FORMAT;

можно было бы еще и остальные выходы за границы проверить

cell._y < _sizes._height && cell._x < _sizes._width
стоило бы вынести в отдельную функцию