HW #2

[Sokolov Viacheslav]

In file included from include/TestRegister.h:4,
                 from test/BoardTest.cpp:2:
test/BoardTest.cpp: In member function ‘virtual void _MAKE_MOVE_AND_CURRENT_PLAYER::_MAKE_MOVE_AND_CURRENT_PLAYER::runTest() const’:
include/Test.h:9:3: error: macro expands to multiple statements [-Werror=multistatement-macros]
    9 |   Test::check((expr), __PRETTY_FUNCTION__, __FILE__, __LINE__);                \
      |   ^~~~
include/Test.h:13:25: note: in expansion of macro ‘CHECK’
   13 | #define CHECK_NOT(expr) CHECK(!(expr))
      |                         ^~~~~
test/BoardTest.cpp:71:9: note: in expansion of macro ‘CHECK_NOT’
   71 |         CHECK_NOT(brd.canMakeMove({i, j}));
      |         ^~~~~~~~~
test/BoardTest.cpp:70:7: note: some parts of macro expansion are not guarded by this ‘if’ clause
   70 |       if (i == 0 && j == 1)
      |       ^~
cc1plus: all warnings being treated as errors
Makefile:31: recipe for target 'obj/test/BoardTest.o' failed
make: *** [obj/test/BoardTest.o] Error 1

[Sokolov Viacheslav]

Просьба сделать так, чтобы стандартный поток вывода в точности соответствовал тому, как описано в условии.
В частности:

• первыми ходят O
• перевод строки перед выводом поля
• Bad move в std::cout
• нет приглашения поиграть еще раз

Makefile:
зачем нужна цель kek?
зачем нужен OBJECTS_APP_WO_MAIN?

WINNING_NUMBER не очень понятное именование ("выигрывающее число")

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

39 void from_string(const std::string &str);

лучше сделать конструктор от field_t, а функцию сделать внешней (прямо в тесте завести вспомогательную функцию), тогда решается проблема непонятных гарантий

clear_cell_content -> reset?

isRow - не очень понятное именование. Неочевидно, что эта функция делает и связана ли как-то с целью игры.

4 #include "Board.h"
5 #include "BoardView?.h"
6 #include "NCursesBoardView.h"
7 #include "StdioBoardView?.h"

все эти включения не нужны в include/BoardController.h, достаточно forward declaration на Board и BoardView?

19 bool isExit() const;

непонятно, что должна делать эта функция (что означает возвращаемое значение)

15 virtual bool getMove(Position &pos) = 0;

​http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-out
лучше сделать virtual std::optional<Position> tryGetMove() = 0;

5 #include "ncurses.h"

не нужно в include/NCursesBoardView.h

9 Position(int row = 0, int col = 0);

1) лучше пометить как explicit - сейчас допустима implicit конвертация из NULL (например)
2) в принципе есть смысл делать такие структуры, если хочется гарантировать иммутабельность, например, но в данном случае выглядит более оправданным
struct Position {int row; int column;}
3) isInRange явно не должна принадлежать Position, потому что сейчас этот класс оказывается связанным с Board. Если хочется этой связанности, то лучше связать их на уровне пространства имен, например, положить Position внутрь Board

8 #define CHECK(expr) \
9 Test::check((expr), PRETTY_FUNCTION, FILE, LINE); \

10 if (!(expr)) \
11 return;

1) лучше поместить внутрь scope {}
2) сейчас на expr нет никаких ограничений - например, это может быть функция, которая меняет состояние обекта, что-нибудь вроде priority_queue.pop(). Из-за этого повторная подставонка expr может приводить к нежелаемому в месте вызова результату. Лучше сделать более явно:
{
const bool result = expr;
check(result,...);
if (!result) return;
}

6 constexpr size_t _size_buffer = (FIELD_SIZE + 1) * (FIELD_SIZE + 1) + 5;

зачем-то нужно?

ALL TESTS ARE PASSED -> ALL TESTS PASSED

2 #include "TestRegister?.h"

не нужен в test/Test.cpp?

4 #define _THIS (*this)

лучше для аналогичных макросов делать #undef, чтобы он случайно куда-то еще не просочился. Так может быть при unity build, например.
А вообще выигрышь от макроса сомнительный.

Board::canMakeMove
игра может быть уже окончена?

bool Board::makeMove
непонятно, почему выбран такой интерфейс, а не более прямолинейный void makeMove (с assert-ом, что его можно сделать)

72 assert(-2 < dpos.getRow() && dpos.getRow() < 2);
73 assert(-2 < dpos.getCol() && dpos.getCol() < 2);

то есть проверяется, что попадает в квадарт от {{-1,-1}, {1,1}} - для этого можно было бы сделать метод (он уже есть на самом деле, но завязан на Board)

runGame
код трудно воспринимать, много if-else. Попробуйте упростить, можно сделать более прямолинейным.

ncurses

45 printw("Do you want to play again?(Y/n)\n");

сообщение сбивает с толку, потому что любая клавиша трактуется как n

можно поддержать и WASD, и стрелочки; про управление можно явно написать в пользовательском интерфейсе

==3820== Memcheck, a memory error detector
==3820== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==3820== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==3820== Command: /home/nicesap/HSE/svn/brilliantov.kirill/hw_02/hw_02
==3820== Parent PID: 2817
==3820==
==3820== Conditional jump or move depends on uninitialised value(s)
==3820== at 0x10A572: operator>>(std::istream&, Position&) (Position.cpp:36)
==3820== by 0x10B414: StdioBoardView::getMove(Position&) (StdioBoardView?.cpp:52)
==3820== by 0x10AE96: BoardController::runGame() (BoardController?.cpp:15)
==3820== by 0x109F53: main (main.cpp:18)
==3820==
==3820==
==3820== HEAP SUMMARY:
==3820== in use at exit: 0 bytes in 0 blocks
==3820== total heap usage: 7 allocs, 7 frees, 81,157 bytes allocated
==3820==
==3820== All heap blocks were freed -- no leaks are possible
==3820==
==3820== For counts of detected and suppressed errors, rerun with: -v
==3820== Use --track-origins=yes to see where uninitialised values come from
==3820== ERROR SUMMARY: 3 errors from 1 contexts (suppressed: 0 from 0)

[Sokolov Viacheslav]

Отсутствует проверка успешности ввода / вывода
(в смысле badbit/failbit у stdin/stdout)

[Sokolov Viacheslav]

По условию Bad move!

7 enum class Mode : int { NORMAL, SILENT };

слишком общее название для глобального пространства имен

 88 bool Board::isInRange(const Position &pos, const Position &leftTopCorner,
 89                       const Position &rightBottomCorner) {
 90   return leftTopCorner.getRow() <= pos.getRow() &&
 91          leftTopCorner.getCol() <= pos.getCol() &&
 92          pos.getRow() < rightBottomCorner.getRow() &&
 93          pos.getCol() < rightBottomCorner.getCol();
 94 }

было бы хорошо отразить асимметричность в названии (Inclusive, Exclusive)

34 is.exceptions(std::ios::failbit | std::ios::badbit);

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