Опять Minesweeper (C++/MFC)

Опубликовано Oct 20, 2012 в Игры, Разбор примеров кода, Хорошие проекты студентов | 5 коммент.



Опять Minesweeper (C++/MFC)

В предыдущем посте про игру “Сапер” был комментарий с предложением варианта решения. Автор переслал его нам. Краткая оценка такая.

Я смотрела только код, связанный с логикой игры (файлы game.h и game.cpp. Все, что связано с интерфейсом на MFC, я комментировать не берусь, ввиду того, что с этой библиотекой никогда не сталкивалась. Достаточно того, что интерфейсная часть работоспособна, а логика игры отделима от интерфейса достаточно прозрачно, что хорошо, и это будет использовано при небольшом рефакторинге кода, о чем и пойдет речь далее.

Общие замечания

0) Стилю написания кода особого внимания не уделялось, как я понимаю. В файлах game.h и game.cpp находится код нескольких разнообразных классов, причем реализация их методов даже не записана по порядку. Имена идентификаторов не единообразны, например, есть имена классов из заглавных букв (классы состояний ячейки CLOSED,  OPENED, MARKED). Вопросы отступов, выравнивания,  наличия пробелов там, где это положено, например, после запятых, были явно пущены на самотек. В целом по этому поводу рекомендуется прочитать, например, “Соглашения по оформлению кода RSDN” и начинать ими пользоваться.

 Общий подход к дизайну логики игры выдержан в объектном стиле, возможно, даже в несколько переусложненном, но по этому коду видно умение применять ООП.

Что я изменила бы

1) Поля класса msw_state (static int msw_state::n_closed;    static int msw_state::n_opened;    static int msw_state::n_marked;) ,  описывающего состояние конкретной клетки, имеют смысл только в контексте конкретной игры (экземпляра класса game). Просто для клетки (экземпляра класса cell) они бессмысленны, вернее одинаковы для всех создаваемых по ходу игры клеток  в каждый конкретный момент. Поэтому их и сделали статическими, а логичнее вообще их перенести в класс game, не объявляя статическими. Кроме того, их объявили публичными, что является нарушением принципа инкапсуляции.

2) Код написан таким образом, что по ходу игры создается и уничтожается большое количество объектов, хранящих статусы ячеек. Таких статусов всего-то имеется три – для их описания достаточно создать по одному объекту классов CLOSED,  OPENED, MARKED и все. А сейчас их создаются и уничтожаются сотни штук – для того, чтобы обеспечить логику подсчета  значений n_closed, n_opened и n_marked, которая в исходном варианте реализована в конструкторах и деструкторах классов статуса. Если вспомнить, что предназначение конструктора – инициализация экземпляра класса, а деструктора – очистка, то идея использовать их для  подсчета чего-либо видится неожиданной.

3) Итак, после перенесения полей n_closed, n_opened и n_marked в класс game, инициализируем их в конструкторе этого класса. Дальше в классе game заводим три поля для описания статусов клеток, инициализируем их, после чего инициализируем сами клетки (массив minefield) со ссылкой на исходный статус CLOSED.

4) В простейших игровых задачах (карты, морской бой и сапер – среди них) при инициализации игры нужно решать задачу генерации случайных перестановок. Предложенный вариант вполне хороший, изменения сделаны, чтобы показать, как убрать лишний вспомогательный массив vector tmp_minefield.

Сображения из пунктов 1) – 4) привели к тому, что наиболее сильно изменился конструктор класса game.

[learn_more  caption = "Конструктор класса  game - первоначальный вариант"]

game::game()
{
msw_state::n_closed=0;
msw_state::n_opened=0;
msw_state::n_marked=0;
in_game=true;
x_cells=9;
y_cells=9;
bombs=10;
 
vector tmp_minefield(x_cells*y_cells-bombs,0);
tmp_minefield.insert(tmp_minefield.end(),bombs,1);
 
srand(unsigned(time(NULL)));
random_shuffle(tmp_minefield.begin(),tmp_minefield.end());
 
 for(int i=0,idx=0;i(minefield).swap(minefield);
 
for_each(minefield.begin(),minefield.end(),
  bind2nd(mem_fun(&cell::on_msg),m_init));
}

[/learn_more]

Конструктор класса game - измененный вариант

game::game()
{
  stateCLOSED = new CLOSED();
  stateOPENED = new OPENED();
  stateMARKED = new MARKED();
 
  n_closed=x_cells*y_cells;
  n_opened=0;
  n_marked=0;
 
  in_game=true;
  x_cells=9;
  y_cells=9;
  bombs=10;
 
  //vector tmp_minefield(x_cells*y_cells-bombs,0);
  //tmp_minefield.insert(tmp_minefield.end(),bombs,1);
 
  for(int i = 0, idx = 0; i < x_cells; ++i)
    for(int j = 0; j < y_cells; ++j, ++idx)
    {
      cell* new_cell;
      if(idx >= x_cells*y_cells-bombs)
      {
        new_cell=new  bomb_cell(this,make_pair(0,0));
      }
      else
      {
        new_cell=new empty_cell(this,make_pair(0,0));
      }
 
      minefield.push_back(new_cell);
    }
 
    srand(unsigned(time(NULL)));
    random_shuffle(minefield.begin(),minefield.end());
 
    for(int i = 0; i < x_cells; ++i)
      for(int j = 0; j < y_cells; ++j)
      {
        minefield[i*x_cells + j]->set_coord(i,j);
      }
 
      for_each(minefield.begin(),minefield.end(),
       bind2nd(mem_fun(&cell::on_msg),m_init));
}

5) По ходу игры в результате манипуляций пользователя статусы клеток будут переключаться просто сменой ссылки на нужный статус, для чего в классе cell изменен метод set_state. Так как смена статуса клетки ведет к изменению итоговых значений для статусов (n_closed, n_opened и n_marked), то в этом же методе они и переcчитываются, а больше n_closed, n_opened и n_marked не могут изменяться нигде. Таким образом, все взаимозависимые изменения и пересчеты собраны в одном единственном месте.

6) Описанные изменения вносились в классы таким образом, чтобы не затронуть сигнатуру методов, на которых построен MFC-интерфейс. Чтобы после сделанных изменений не возникли проблемы с правами доступа, классу game пришлось подарить дружбу другим классам, для чего в его заголовок добавили следующий код

   friend class cell;
   friend class CLOSED;
   friend class OPENED;
   friend class MARKED;

Это все. Если автор не выступит с возражениями – через некоторое время я выложу оба варианта кода целиком. 


Автор публикации:

5 Коммент. : “Опять Minesweeper (C++/MFC)”

  1. Автор Игры says:

    Здравствуйте Галина.

    Огромное спасибо за уделенное внимание, и время по моему вопросу.

    Для начала расскажу о ходе разработки этого проекта.

    Естественно сразу просматривается два класса – cell -ячейка, и game – менеджер игры.
    При анализе проекта, помимо основной задачи – реализации логики игры, были поставлены следующие условия:

    1. Избавиться от связи < <все со всеми>> между объектами cell.

    Для этого менеджер-game выступает в роли наблюдателя, а общение между объектами сводится к передаче сообщений к менеджеру. Хотя как мне сейчас кажется, классическая реализация этого паттерна была бы удачней.

    2. В виду того, что у cell имеется несколько состояний, от которых
    зависит поведение объекта, а также переходы из одного состояния в другое неоднозначны, необходимо что-то предпринять, чтобы избавиться от многоуровневых if-операторов, разбросанных по всему коду.
    Для этого вводится конечный автомат, где класс msw_state его абстракция , а cell::state – состояние.

    3. Сделать логику игры если не кросс платформенной, то хотя бы cross-framework.

    После того, как вам был отправлен проект, я попытался перенести его в Qt, что было моим первым знакомством с этой библиотекой. В ходе этого эксперимента были выявлены некоторые ошибки в подходе к проектированию. В итоге доработки framework работает с классом game, а game в свою очередь с потомком от класса painter(MFC_painter, QT_painter).

    По вашим замечаниям:

    0) согласен;
    1) согласен, решение не разрушающее архитектуру проекта найдено, см.п.5)
    2) здесь я вижу один < <минус>> – создание и удаление множества объектов.
    < <Плюс>> – прозрачный алгоритм, < <состояние>> изменяет само < <состояние>> а не менеджер, нет зависимостей и сильных связей между классами.
    Кроме того размер объекта(нет членов-данных), и его создание не несут существенных расходов.
    3) см.п. 5)
    4) < <Плюс>> – нет вспомогательного массива, < <минус>> – cell инициализируется в три прохода.
    5) и 6) совсем не согласен:
    – по замыслу клетка(cell) ни чего не знает о своем состоянии, а именно статус(msw_state) управляет клеткой вызовом on_open(). Т.е.
    клетке совершенно не нужно подсчитывать статусы(n_closed, n_opened и
    n_marked) при попытке изменить состояние. Тем более, что состояние может не измениться, например -переход из помеченного состояния в открытое.
    - friend class – это не та дружба, что нам нужна.
    - решение – отказаться от статусов (n_closed и n_marked).
    Условие победы: int game::n_opened==”всего_клеток – количество_бомб”, n_opened инкрементируется при открытии клетки.

  2. Галина says:
      A) Реализация замечания из пункта 1 + удаление n_closed и n_marked – хороший ход, согласна.
      Б) Соображения по поводу минимизации связей между классами полностью правильные, к этому, разумеется, нужно стремиться.

    ОПРЕДЕЛЕНИЕ ЗАВИСИМОСТИ (из Википедии и пр.)

    “Зависимостью называется отношение использования, согласно которому изменение в спецификации одного элемента (например, класса «cell») может повлиять на использующий его элемент (класс «CLOSED»). Часто зависимости показывают, что один класс использует другой в качестве аргумента.”

    Но все же полностью избежать зависимостей между ячейкой и статусом не вышло, так как класс cell зависит от класса msw_state и через него от класса CLOSED.

    class cell
    {
    protected:
        game*     pMngr;
        msw_state* state;
        coord cell_coord;
    public:
    ************
     
    // состояние ячейки
        void opened(){state->opened(this);}//ЗДЕСЬ ЗАВИСИМОСТЬ
    ************
    }

    если изменить, например, сигнатуру метода CLOSED::opened(cell* cell), то нужно делать и изменения класса cell и, наоборот, CLOSED зависит от cell

    void CLOSED::opened(cell* cell) //прямо использует в качестве аргумента
    {
     cell->set_state(new OPENED());
     cell->on_open();
    }

    Разумеется, ваш подход, выбранный для изменений статусов, направлен на минимизацию зависимостей (это плюс), а то, что я предложила в пункте 5 их усиливает, вернее делает их более сложными и это, согласна, минус. С другой стороны, в моей реализации (ВАРИАНТ 2) создаются всего четыре объекта, а в вашей (ВАРИАНТ 1) – сотни. Что лучше в целом – более сильные зависимости или интенсивное перераспределение памяти – об этом можно дискутировать, наверное, долго. В реальной жизни ВАРИАНТ 2 вполне могло стать необходимым реализовать, если бы нужно было бы устранять проблемы производительности. А пока хорошо, что

      а) есть два варианта реализации, которые можно сравнивать.
      б) все было вами спроектировано так, что поменять реализацию ВАРИАНТа 1 на ВАРИАНТ2 оказалось достаточно легко, так как потребовалось небольшое количество изменений, места которых были прозрачными даже для другого разработчика (мало зависимостей). Кроме того, добавление нового класса EXPLODED никак не затронуло логику этих изменений.
      в) По поводу ДРУЖБЫ. Дружба, да еще между классами, – это тоже минус, согласна, так как нарушает инкапсуляцию. Но она объявлялась, чтобы решить проблемы доступа по-быстрому. Правильное решение – написать необходимые функции для доступа к полям (getters). Это и сделано в окончательной версии кода, которую я загружу, вместе с вашим вторым вариантом.
  3. Большое спасибо за ответ.
    К сожалению в посте http://www.fulcrumweb.com.ua/archives/4908 две ссылки на
    один архив.

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

    //CellState.h
    class CellState    //состояние ячейки
    {
    public:
     
        static CellState* defaultState();    
                                               //переходы состояний,
        virtual void opened(Cell* cell){}      //ячейка открыта
        virtual void marked(Cell* cell){}      //ячейка помечена
        virtual void uncovered(Cell* cell){}   //ячейка открыта в случае проигрыша
        virtual void markClosed(Cell* cell){}  //домаркировать закрытые, в случае победы
        virtual void exploded(Cell* cell){}
     
        virtual void draw(coord c, Painter* pp);
     
    protected:
        CellState(){}
        virtual ~CellState(){}
     
        static ClosedState*   getClosed();
        static OpenedState*   getOpened();
        static MarkedState*   getMarked();
        static ExplodedState* getExploded();
    };
    .......................
    //CellState.cpp
    CellState* CellState::defaultState()
    {
    	return getClosed();
    }    
    ClosedState* CellState::getClosed()
    {
    	static ClosedState obj;
    	return &obj;
    }
     
    void ClosedState::opened(Cell* cell)
    {
    	cell->setState(getOpened());
    	cell->onOpen();
    }
    ........................
    //Cell.cpp
     
    Cell::Cell(Game*  pM,coord cr):pMngr(pM),cellCoord(cr)
    {
    	state = CellState::defaultState();
    }
     
    void Cell::setState(CellState* s)
    { 
        state = s; 
    }

Оставить комментарий

Ваш адрес email не будет опубликован.


*