HW #2

[Sokolov Viacheslav]

==18011== Memcheck, a memory error detector
==18011== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==18011== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==18011== Command: /home/nicesap/HSE/svn/morozov.aleksandr/hw_02/hw_02
==18011== Parent PID: 17999
==18011==
==18011==
==18011== HEAP SUMMARY:
==18011== in use at exit: 24 bytes in 1 blocks
==18011== total heap usage: 22 allocs, 21 frees, 81,208 bytes allocated
==18011==
==18011== 24 bytes in 1 blocks are definitely lost in loss record 1 of 1
==18011== at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18011== by 0x109F48: main (main.cpp:15)
==18011==
==18011== LEAK SUMMARY:
==18011== definitely lost: 24 bytes in 1 blocks
==18011== indirectly lost: 0 bytes in 0 blocks
==18011== possibly lost: 0 bytes in 0 blocks
==18011== still reachable: 0 bytes in 0 blocks
==18011== suppressed: 0 bytes in 0 blocks
==18011==
==18011== For counts of detected and suppressed errors, rerun with: -v
==18011== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

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

• первыми ходят O
• перевод строки перед выводом поля
• для приглашения к игре поле выводится один раз, а не по разу на (неуспешный) ввод

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

3 const int FIELD_SIZE = 10;

-> constexpr

23 int _field[FIELD_SIZE][FIELD_SIZE]; 0 - empty, 1 - X, 2 - O

лучше использовать std::array (проверит выход за границы), а также
хранить не int, а enum (CellContent? ?)

15 void move(int x, int y, Player player);

если Player поддерживается внутри Board, то зачем его передавать снаружи в этом методе?

кажется, лучше именовать
init -> reset

Player::NOONE может быть невалиден в некоторых контекстах, может быть, лучше использовать std::optional, чтобы точнее выразить, что именно нужно в конкретном контексте?

include/BoardTest.h
какая мотивация не отделать объявление от определения (здесь и в других файлах)?

9 Board::Player X = Board::X;

10 Board::Player O = Board::O;

static, constexpr?

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

не нужны в include/GameController.h
Достаточно forward declaration на BoardView?

4 #include <ncurses.h>

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

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

test/RunTests.cpp:
возможно, стоит возвращать не 0, если тесты не пройдены

Названия тестов не очень лаконичные:
что такое testIntegrate - непонятно
Вместо 1,2 лучше что-нибудь осмысленное написать

28 _player = (Board::Player)(3 - (int)_player);
лучше вынести в именованую функцию, и реализовать не закладываясь на значения перечисления

checkWin -> updateState?

36 int ROW_TO_WIN = 5;

constexpr; лучше вынести на уровень класса

44 for (int dir = 0; dir < (int)dx.size(); ++dir) {

не лучше dir сделать size_t?

x < 0

y < 0

x >= FIELD_SIZE

y >= FIELD_SIZE

стоит вынести в отдельную функцию, используется в двух местах

Сейчас логика модификации Board дублируется между View. В условии предполагается делать иначе: на View ответственность лишь за взаимодействие с пользователем (ввод / вывод), а модификацией Board занимается Controller. Это позволяет проще добавлять новые способы взаимодействия. Единственность ответственности - важный принцип, позволяющий строить надежные системы в том числе за счет конкретных гарантий и понятности процесса тестирования.

Отсутствует проверка успешности ввода / вывода (в смысле failbit/badbit у std::cin/cout)

[Alexander Morozov]

Убрал утечку, теперь удаляю view (пришлось добавить виртуальный деструктор)

Поправил формат вывода

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

Заменил const на constexpr.

Заменил _field на std::array, содержащий Player.

Убрал player из move. Изначально это было написано в условии задания, и я подумал, что хочется иметь дополнительную проверку, храня player с обеих сторон.

Переименовал init в reset.

Мне кажется, в данной программе NOONE нигде не может вылезти, кроме как там, где имеется в виду именно он. На всякий случай, добавил assert в Board::move.

В условии был пример реализации теста, там он реализован в .h файле. Я подумал, что это логично, ведь отделять нужно, чтобы можно было подключать заголовочный файл в нескольких файлах, но в данном случае он нам нужен только в главном файле со всеми тестами. А реализовывать там же, где объявление, удобнее.

Убрал лишний include в GameController?.h

Убрал лишний include в NcursesBoardView?.h, сначала даже не подумал о том, что он может быть лишним.

Добавил enum Mode

Возвращаю 1, если не все тесты прошли.

Переименовал тесты.

Добавил функцию otherPlayer.

Переименовал checkWin, вынес ROW_TO_WIN в Board.h.

Теперь модификацией Board занимается только контроллер, но View все еще читает данные из Board.

Какая-то проверка для ввода уже была, улучшил и добавил проверку для cout. Выхожу, если не получилось вывести, потому что это уже довольно странная ситуация.

[Sokolov Viacheslav]

В curses режиме получаю
.......... hw_02: src/Board.cpp:38: void Board::move(int, int): Assertion `canMove(x, y)' failed.

fish: “./hw_02 curses” terminated by signal SIGABRT (Abort)

⏎

если игра завершилась по (-1, -1), поле печатать не нужно

9 std::pair<int, int> processTurn() override;

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

реализовывать там же, где объявление, удобнее.

в момент реализации - возможно. А вот читать такой код - неудобно. Удобно разделять намерение и реализацию, зачастую реализацию смотреть не нужно, если хорошо выражено намерение.