HW#2

[Sokolov Viacheslav]

╰─>$ make
mkdir -p obj
g++ -O2 -g -Wall -Werror -Wextra -std=c++11 -Iinclude -fsanitize=address -c -MMD -o obj/NcursesBoardView.o src/NcursesBoardView.cpp
src/NcursesBoardView.cpp: In function ‘unsigned int colorByChar(char)’:
src/NcursesBoardView.cpp:61:18: error: ‘assert’ was not declared in this scope

61 | default: assert(0 && "Invalid char");

| ^~

src/NcursesBoardView.cpp:2:1: note: ‘assert’ is defined in header ‘<cassert>’; did you forget to ‘#include <cassert>’?

1 | #include "NcursesBoardView?.h"

+++ |+#include <cassert>

2 | #include <ncurses.h>

src/NcursesBoardView.cpp: In member function ‘virtual void NcursesBoardView::endGame(Board::Enums::GameState?)’:
src/NcursesBoardView.cpp:140:5: error: ‘assert’ was not declared in this scope

140 | assert(g != Enums::GameState::IN_PROGRESS);

| ^~

src/NcursesBoardView.cpp:140:5: note: ‘assert’ is defined in header ‘<cassert>’; did you forget to ‘#include <cassert>’?
src/NcursesBoardView.cpp: In function ‘unsigned int colorByChar(char)’:
src/NcursesBoardView.cpp:61:9: error: control reaches end of non-void function [-Werror=return-type]

61 | default: assert(0 && "Invalid char");

|

cc1plus: all warnings being treated as errors
Makefile:24: recipe for target 'obj/NcursesBoardView.o' failed
make: * [obj/NcursesBoardView.o] Error 1

[Sokolov Viacheslav]

На самом деле отсутствует больше заголовочных файлов. Желательно Вам найти способ компиляции, при котором заголовочные файлы стандартной библиотеки не подключается автоматически (видимо, с помощью precompiled header)

[Igor Engel]

Разобрался кажется. Проблема в том, что на MacOS по умолчанию стоит clang (и при этом, вызывается по gcc/g++). Сейчас вроде прокомпилировал всё через нормальный g++.

[Sokolov Viacheslav]

Может быть, вместо Board::Enums сделать пространство имен? Этот класс используется как пространство имен, но при этом вложен в класс Board. Если отказаться от вложенности, можно будет использовать именно namespace.

BoardView?:
возможно, стоит вместо std::pair<int, int> завести специальный тип (например, struct Turn{int x; int y;}), чтобы в наследниках не ошибиться с трактовкой, какая из координат за что отвечает

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

В разных заголовочных файлах private/public секции выравниваются по-разному

Кажется, silent - это свойство StdioBoardView?, а не контроллера

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

На мой вкус, макрос ID не повышает читаемость, а скорее наоборот

Можно придумать универсальный пример ничьей, работающий и для досок размером больше, чем 10x10 (остатки по модулю 3 помогут).

Вместо 100 лучше было бы использовать константу, зависящую от размера поля.

Лучше не использовать assert для внешнего тестирования: сам процесс тестирования должен быть выстроен таким образом, чтобы тестирующий код не завершался аварийно. При этом проверки конечно же полезно делать. Рекомендую посмотреть на макросы ASSERT из gtest.

Вместо тестов FIELD_ELEM_X / FIELD_ELEM_O можно было бы сделать просто проверку состояния всего поля .field() == ...

repr возвращает либо char, либо string. Возможно, стоит сделать так, чтобы возвращаемый тип был ясен из имени функции, чтобы не ошибиться при использовании (в шаблонном коде это могло бы быть crucial)

CHECK_MAX_STREAK вполне можно вынести в функцию. Здесь макрос используется не по делу.
Проверки "5 в ряд" можно делать с помощью цикла по направлению. Константу 4 можно было бы связать с константой "сколько в ряд до победы", чтобы легко кастомизировать игру.

Сейчас считывание в StdioBoardView? работает не так, как требуется в задании:
1) ввод 0-0 будет принят, хотя не должен
2) ввод

1

2
будет принят, хотя не должен
3) неправильно обрабатывает неожиданный eof / bad

Не обрабатываются ошибки вывода

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

10 #define COLOR_E 0
11 #define COLOR_O 1
12 #define COLOR_X 2

для этих целей в c++ есть static constexpr.
Макросы не являются предпочтительным средством языка.

#undef происходит не для всего.
(может быть важно в unity build)

Duplicate from Board::isInField
почему просто этот метод не использовать?

В curses режиме неудобно смотреть состояние поля, потому что при повторной постановке курсора в уже занятую позицию в ней отображается знак очередного игрока, а не тот знак, который реально находится в позиции

[Sokolov Viacheslav]

Разобрался кажется. Проблема в том, что на MacOS по умолчанию стоит clang (и при этом, вызывается по gcc/g++). Сейчас вроде прокомпилировал всё через нормальный g++.

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

[Igor Engel]

Всё-таки хочется вложенность, так-как это именно enum'ы относящиеся к Board (можно, конечно, назвать BoardEnums?, но со вложенностью выглядит логичнее)

Fixed

Fixed

Fixed

Всё-таки кажется контроллера, просто несовместимое конкретно с curses

Fixed. Ну, и режим тогда точно свойство контроллера.

Ок

TODO

Fixed

Fixed

Fixed

Fixed

Fixed

Fixed

Fixed

Fixed

Fixed

Fixed

Fixed

TODO

[Sokolov Viacheslav]

Просьба внимательнее относиться к version

[Sokolov Viacheslav]

g++: error: unrecognized command line option ‘-Weverything’
Makefile:25: recipe for target 'obj/NcursesBoardView.o' failed
make: * [obj/NcursesBoardView.o] Error 1

[Sokolov Viacheslav]

​https://quuxplusone.github.io/blog/2018/12/06/dont-use-weverything/

[Sokolov Viacheslav]

Относиться к != располагать одно в другом. В языке есть и другие средства, чтобы подчеркнуть связь между сущностями, например, пространства имен. Можно сделать
namespace tictactoe и положить внутрь Board, Player, GameState?, опционально FieldElem?.
Так, "игрок" - это не свойство доски. Технически глубокая вложенность неудобна, хоть и не запрещена:

Board::Enums::isWin(Board::Enums::GameState::IN_PROGRESS) - достаточно многословно.

Ну и все еще struct vs namespace
​https://stackoverflow.com/questions/2614314/when-to-use-a-namespace-or-a-struct (тема не раскрыта, на мой взгляд)

48 void makeTurn(int y, int x, Enums::Player player);
Поскольку Board отвечает за текущего игрока, его можно здесь не передавать (лучше исключить из интерфейса)

2 #include "Board.h"
3 #include "BoardView?.h"

избыточны в include/GameController.h, достаточно forward declaration

2 #include <ncurses.h>

избыточен в include/NcursesBoardView.h

139 Board::Enums::FieldElem? target = elemAt(y, x);
assert(isPlayer(target))

std::cout не проверяется на успешность вывода. Кажется, лучше обрабатывать исключительные ситуации (с потоком что-то не так) с помощью исключений.

Может быть удобно использовать std::stringstream

Ввод 0 0 bad будет обработан не так, как того требует условие

[Igor Engel]

Ну. Допустим, сделал так. Не сказать что вложенность уменьшилась... Можно уменьшить заменив enum class на enum, но тогда получается не слишком типобезопасно. Ещё можно методы вытащить в чистый tictactoe, но тогда будет совсем мешанина...

...

makeTurn: Ок

Fixed

Fixed

Fixed

Fixed

Не совсем понятно. getline в него сделать нельзя, так-что от промежуточной строки не избавиться. И тогда istringstream вроде лучше передаёт предназначение.

Вроде правильно, bad move.

[Sokolov Viacheslav]

╰─>$ make
g++-9 -O2 -g -Wall -Werror -Wextra -std=c++11 -Iinclude -fsanitize=address -c -MMD -o obj/test/BoardTest.o test/BoardTest.cpp
In file included from test/BoardTest.cpp:3:
test/BoardTest.cpp: In member function ‘virtual void BoardTest::runAllTests()’:
test/BoardTest.cpp:199:14: error: ‘FIELD_ELEM_CORRESPONDS_TO_TURNS_test’ was not declared in this scope
  199 |     RUN_TEST(FIELD_ELEM_CORRESPONDS_TO_TURNS);
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/TestImplMacro.h:3:24: note: in definition of macro ‘RUN_TEST’
    3 | #define RUN_TEST(name) name ## _test()
      |                        ^~~~
test/BoardTest.cpp: At global scope:
test/BoardTest.cpp:142:10: error: ‘void {anonymous}::FIELD_CORRESPONDS_TO_TURNS_test()’ defined but not used [-Werror=unused-function]
  142 |     TEST(FIELD_CORRESPONDS_TO_TURNS) {
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/TestImplMacro.h:2:25: note: in definition of macro ‘TEST’
    2 | #define TEST(name) void name ## _test()
      |                         ^~~~
cc1plus: all warnings being treated as errors
Makefile:30: recipe for target 'obj/test/BoardTest.o' failed
make: *** [obj/test/BoardTest.o] Error 1

[Sokolov Viacheslav]

==9103== Memcheck, a memory error detector
==9103== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==9103== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==9103== Command: /home/nicesap/HSE/svn/engel.igor/hw_02/hw_02
==9103== Parent PID: 9102
==9103== 
==9103== Conditional jump or move depends on uninitialised value(s)
==9103==    at 0x10ADDE: isInField (Board.cpp:97)
==9103==    by 0x10ADDE: tictactoe::Board::isValidTurn(int, int) const (Board.cpp:107)
==9103==    by 0x10A859: GameController::tryTurn(int, int) (GameController.cpp:31)
==9103==    by 0x10A8FE: GameController::runGame() (GameController.cpp:20)
==9103==    by 0x109F5E: main (main.cpp:39)
==9103== 
==9103== Conditional jump or move depends on uninitialised value(s)
==9103==    at 0x10A85F: GameController::tryTurn(int, int) (GameController.cpp:31)
==9103==    by 0x10A8FE: GameController::runGame() (GameController.cpp:20)
==9103==    by 0x109F5E: main (main.cpp:39)
==9103== 
==9103== Conditional jump or move depends on uninitialised value(s)
==9103==    at 0x10A901: GameController::runGame() (GameController.cpp:20)
==9103==    by 0x109F5E: main (main.cpp:39)
==9103== 
==9103== 
==9103== HEAP SUMMARY:
==9103==     in use at exit: 0 bytes in 0 blocks
==9103==   total heap usage: 4 allocs, 4 frees, 80,912 bytes allocated
==9103== 
==9103== All heap blocks were freed -- no leaks are possible
==9103== 
==9103== For counts of detected and suppressed errors, rerun with: -v
==9103== Use --track-origins=yes to see where uninitialised values come from
==9103== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

[Sokolov Viacheslav]

 26     namespace FieldElem {
 27         enum class value {
 28                 EMPTY,
 29                 O,
 30                 X
 31         };
 32     }

не стоит мешать разный размер отступов

Удобно, кода из codestyle следует, имеем мы дело с пространство имен или именем класса, глядя только на название. Сейчас смешаны разные стили:

• namespace tictactoe
• namespace Player
• enum class value
• class Board

Должно быть как-то так:
34 if(s.eof() && not s.fail()) {

чтобы корректно обработать 0 0a

кроме того, по условию вот это 0 0 тоже является корректным ходом