Тетрис:Расположение классов

Question

Я написал рабочий клон tetris, но у него довольно беспорядочный макет.Могу ли я, пожалуйста, получить отзыв о том, как реструктурировать мои классы, чтобы улучшить мое кодирование.Я фокусируюсь на том, чтобы сделать свой код как можно более универсальным, пытаясь сделать его в большей степени движком для игр, использующих только блоки.

Каждый блок создается в игре отдельно.В моей игре есть 2 списка блокировок (связанных списка):Статические блоки и Тетроид.StaticBlocks - это, очевидно, список всех неподвижных блоков, а tetroid - это 4 блока текущего tetroid.

1. В основном создается Мир.

2. Сначала создается новый тетроид (4 блока в списке тетроидов) с помощью (NewTetroid)

3. Столкновение обнаруживается функциями (***Collide) путем сравнения каждого тетроида со всеми статическими блоками с использованием функций (If*****).

4. Когда tetroid останавливается (достигает дна / блокируется), он копируется (CopyTetroid) в StaticBlocks, и Tetroid становится пустым, затем выполняются тесты для полных строк, блоки уничтожаются / отбрасываются и т.д. Путем поиска StaticBlocks с помощью (SearchY).

5. Создается новый тетроид.

(TranslateTetroid) и (RotateTetroid) выполняют операции над каждым блоком в списке Tetroid одну за другой ( Я думаю, что это плохая практика).

(DrawBlockList) просто просматривает список, запуская функцию Draw() для каждого блока.

Вращение управляется путем установки оси вращения относительно первого блока в Tetroid при вызове (NewTetroid).Моя функция вращения (Rotate) для каждого блока поворачивает его вокруг оси, используя вход + -1 для поворота влево / вправо.Режимы вращения и состояния предназначены для блоков, которые вращаются 2 или 4 различными способами, определяя, в каком состоянии они находятся в данный момент, и следует ли их поворачивать влево или вправо. Я не доволен тем, как они определены в "World", но я не знаю, куда их поместить, сохраняя при этом мою функцию (Rotate) общей для каждого блока.

Мои занятия заключаются в следующем

class World
{
    public:
    /* Constructor/Destructor */
    World();
    ~World();

    /* Blocks Operations */
    void AppendBlock(int, int, BlockList&);
    void RemoveBlock(Block*, BlockList&);;

    /* Tetroid Operations */
    void NewTetroid(int, int, int, BlockList&);
    void TranslateTetroid(int, int, BlockList&);
    void RotateTetroid(int, BlockList&);
    void CopyTetroid(BlockList&, BlockList&);

    /* Draw */
    void DrawBlockList(BlockList&);
    void DrawWalls();

    /* Collisions */
    bool TranslateCollide(int, int, BlockList&, BlockList&);
    bool RotateCollide(int, BlockList&, BlockList&);
    bool OverlapCollide(BlockList&, BlockList&); // For end of game

    /* Game Mechanics */
    bool CompleteLine(BlockList&); // Test all line
    bool CompleteLine(int, BlockList&); // Test specific line
    void ColourLine(int, BlockList&);
    void DestroyLine(int, BlockList&);
    void DropLine(int, BlockList&); // Drops all blocks above line

    int rotationAxisX;
    int rotationAxisY;
    int rotationState; // Which rotation it is currently in
    int rotationModes; // How many diff rotations possible

    private:
    int wallX1;
    int wallX2;
    int wallY1;
    int wallY2;
};

class BlockList
{
    public:
    BlockList();
    ~BlockList();

    Block* GetFirst();
    Block* GetLast();

    /* List Operations */
    void Append(int, int);
    int  Remove(Block*);
    int  SearchY(int);

    private:
    Block *first;
    Block *last;
};

class Block
{
    public:
    Block(int, int);
    ~Block();

    int GetX();
    int GetY();

    void SetColour(int, int, int);

    void Translate(int, int);
    void Rotate(int, int, int);

    /* Return values simulating the operation (for collision purposes) */
    int IfTranslateX(int);
    int IfTranslateY(int);
    int IfRotateX(int, int, int);
    int IfRotateY(int, int, int);

    void Draw();

    Block *next;

    private:
    int pX; // position x
    int pY; // position y
    int colourR;
    int colourG;
    int colourB;
};

Извините, если это немного неясно или многословно, я просто ищу помощи в реструктуризации.

Answer 1

• Что такое одинокий ответственность за World класс?Это просто большой двоичный объект, содержащий практически все виды функциональности.Это не очень хороший дизайн.Одна из очевидных обязанностей заключается в том, чтобы "представлять сетку, на которую помещены блоки".Но это не имеет ничего общего с созданием тетроидов, манипулированием списками блоков или рисованием.На самом деле, большая часть этого, вероятно, вообще не обязательно должна быть в классе.Я бы ожидал, что World объект, содержащий BlockList вы вызываете StaticBlocks, чтобы он мог определить сетку, на которой вы играете.
• Почему вы определяете свой собственный Blocklist?Вы сказали, что хотите, чтобы ваш код был универсальным, так почему бы не разрешить Любой контейнер, который будет использоваться?Почему я не могу использовать std::vector<Block> если я захочу?Или a std::set<Block>, или какой-нибудь контейнер для домашнего приготовления?
• Используйте простые имена, которые не дублируют информацию и не противоречат самим себе. TranslateTetroid не переводит тетроид.Он переводит все блоки в список блоков.Так и должно быть TranslateBlocks или что-то в этомроде.Но даже это излишне.Мы можем видеть из подписи (для этого требуется BlockList&) что это работает с блоками.Так что просто назови это Translate.
• Старайтесь избегать комментариев в стиле C (/*...*/).C++-стиль (//..) ведет себя немного приятнее в том смысле, что если вы используете комментарий в стиле C для целого блока кода, он сломается, если этот блок также содержит комментарии в стиле C.(В качестве простого примера, /*/**/*/ не сработает, так как компилятор увидит первый */ как конец комментария, так и последний */ не будет рассматриваться как комментарий.
• Что со всеми этими (неназванными) int параметры?Это делает ваш код невозможным для чтения.
• Уважайте особенности языка и условности.Способ скопировать объект - это использовать его конструктор копирования.Так что вместо того, чтобы CopyTetroid функция, дающая BlockList конструктор копирования.Затем, если мне нужно скопировать один из них, я могу просто сделать BlockList b1 = b0.
• Вместо того , чтобы void SetX(Y) и Y GetX() методы, отбросьте избыточный префикс Get / Set и просто имейте void X(Y) и Y X().Мы знаем, что это геттер, потому что он не принимает параметров и возвращает значение.И мы знаем, что другой является установщиком, потому что он принимает параметр и возвращает void .
• BlockList это не очень хорошая абстракция.У вас очень разные потребности в "текущем тетроиде" и "списке статических блоков, находящихся в данный момент в сетке".Статические блоки могут быть представлены простой последовательностью блоков, как у вас (хотя последовательность строк или 2D-массив могут быть более удобными), но активному в данный момент тетроиду требуется дополнительная информация, такая как центр вращения (который не принадлежит World).
  • Простой способ представить тетроид и облегчить вращение может заключаться в том, чтобы блоки-элементы сохраняли простое смещение от центра вращения.Это упрощает вычисление вращений и означает, что блоки-члены вообще не нужно обновлять во время перевода.Просто центр вращения должен быть перемещен.
  • В статическом списке блокам даже неэффективно знать свое местоположение.Вместо этого сетка должна сопоставлять местоположения с блоками (если я спрошу сетку "какой блок существует в ячейке (5,8), он должен быть в состоянии вернуть блок.но самому блоку не нужно хранить координату.Если это произойдет, это может стать головной болью при обслуживании.Что, если из-за какой-то незначительной ошибки два блока в конечном итоге будут иметь одну и ту же координату?Это может произойти, если блоки хранят свои собственные координаты, но не в том случае, если сетка содержит список того, какой блок где находится.)
  • это говорит нам о том, что нам нужно одно представление для "статического блока" и другое для "динамического блока" (оно должно сохранять смещение от центра тетроида).Фактически, "статический" блок может быть сведен к основному:Либо ячейка в сетке содержит блок, и этот блок имеет цвет, либо она не содержит блока.С этими блоками больше не связано поведение, поэтому, возможно, вместо этого следует смоделировать ячейку, в которую они помещены.
  • и нам нужен класс, представляющий подвижный / динамический тетроид.
• Поскольку большая часть вашего обнаружения столкновений является "прогностической" в том смысле, что она имеет дело с вопросом "что, если бы я переместил объект сюда", возможно, было бы проще реализовать немутирующие функции перевода / поворота.Они должны оставить исходный объект неизмененным, а повернутую / переведенную копию вернуть.

Итак, вот первый проход по вашему коду, простое переименование, комментирование и удаление кода без слишком большого изменения структуры.

class World
{
public:
    // Constructor/Destructor
    // the constructor should bring the object into a useful state. 
    // For that, it needs to know the dimensions of the grid it is creating, does it not?
    World(int width, int height);
    ~World();

    // none of thes have anything to do with the world
    ///* Blocks Operations */
    //void AppendBlock(int, int, BlockList&);
    //void RemoveBlock(Block*, BlockList&);;

    // Tetroid Operations
    // What's wrong with using BlockList's constructor for, well, constructing BlockLists? Why do you need NewTetroid?
    //void NewTetroid(int, int, int, BlockList&);

    // none of these belong in the World class. They deal with BlockLists, not the entire world.
    //void TranslateTetroid(int, int, BlockList&);
    //void RotateTetroid(int, BlockList&);
    //void CopyTetroid(BlockList&, BlockList&);

    // Drawing isn't the responsibility of the world
    ///* Draw */
    //void DrawBlockList(BlockList&);
    //void DrawWalls();

    // these are generic functions used to test for collisions between any two blocklists. So don't place them in the grid/world class.
    ///* Collisions */
    //bool TranslateCollide(int, int, BlockList&, BlockList&);
    //bool RotateCollide(int, BlockList&, BlockList&);
    //bool OverlapCollide(BlockList&, BlockList&); // For end of game

    // given that these functions take the blocklist on which they're operating as an argument, why do they need to be members of this, or any, class?
    // Game Mechanics 
    bool AnyCompleteLines(BlockList&); // Renamed. I assume that it returns true if *any* line is complete?
    bool IsLineComplete(int line, BlockList&); // Renamed. Avoid ambiguous names like "CompleteLine". is that a command? (complete this line) or a question (is this line complete)?
    void ColourLine(int line, BlockList&); // how is the line supposed to be coloured? Which colour?
    void DestroyLine(int line, BlockList&); 
    void DropLine(int, BlockList&); // Drops all blocks above line

    // bad terminology. The objects are rotated about the Z axis. The x/y coordinates around which it is rotated are not axes, just a point.
    int rotationAxisX;
    int rotationAxisY;
    // what's this for? How many rotation states exist? what are they?
    int rotationState; // Which rotation it is currently in
    // same as above. What is this, what is it for?
    int rotationModes; // How many diff rotations possible

private:
    int wallX1;
    int wallX2;
    int wallY1;
    int wallY2;
};

// The language already has perfectly well defined containers. No need to reinvent the wheel
//class BlockList
//{
//public:
//  BlockList();
//  ~BlockList();
//
//  Block* GetFirst();
//  Block* GetLast();
//
//  /* List Operations */
//  void Append(int, int);
//  int  Remove(Block*);
//  int  SearchY(int);
//
//private:
//  Block *first;
//  Block *last;
//};

struct Colour {
    int r, g, b;
};

class Block
{
public:
    Block(int x, int y);
    ~Block();

    int X();
    int Y();

    void Colour(const Colour& col);

    void Translate(int down, int left); // add parameter names so we know the direction in which it is being translated
    // what were the three original parameters for? Surely we just need to know how many 90-degree rotations in a fixed direction (clockwise, for example) are desired?
    void Rotate(int cwSteps); 

    // If rotate/translate is non-mutating and instead create new objects, we don't need these predictive collision functions.x ½
    //// Return values simulating the operation (for collision purposes) 
    //int IfTranslateX(int);
    //int IfTranslateY(int);
    //int IfRotateX(int, int, int);
    //int IfRotateY(int, int, int);

    // the object shouldn't know how to draw itself. That's building an awful lot of complexity into the class
    //void Draw();

    //Block *next; // is there a next? How come? What does it mean? In which context? 

private:
    int x; // position x
    int y; // position y
    Colour col;
    //int colourR;
    //int colourG;
    //int colourB;
};

// Because the argument block is passed by value it is implicitly copied, so we can modify that and return it
Block Translate(Block bl, int down, int left) {
    return bl.Translate(down, left);
}
Block Rotate(Block bl, cwSteps) {
    return bl.Rotate(cwSteps);
}

Теперь давайте добавим некоторые недостающие фрагменты:

Во-первых, нам нужно будет представить "динамические" блоки, тетроид, владеющий ими, и статические блоки или ячейки в сетке.(Мы также добавим простой метод "Collides" в класс world / grid)

class Grid
{
public:
    // Constructor/Destructor
    Grid(int width, int height);
    ~Grid();

    // perhaps these should be moved out into a separate "game mechanics" object
    bool AnyCompleteLines();
    bool IsLineComplete(int line);
    void ColourLine(int line, Colour col);Which colour?
    void DestroyLine(int line); 
    void DropLine(int);

    int findFirstInColumn(int x, int y); // Starting from cell (x,y), find the first non-empty cell directly below it. This corresponds to the SearchY function in the old BlockList class
    // To find the contents of cell (x,y) we can do cells[x + width*y]. Write a wrapper for this:
    Cell& operator()(int x, int y) { return cells[x + width*y]; }
    bool Collides(Tetroid& tet); // test if a tetroid collides with the blocks currently in the grid

private:
    // we can compute the wall positions on demand from the grid dimensions
    int leftWallX() { return 0; }
    int rightWallX() { return width; }
    int topWallY() { return 0; }
    int bottomWallY { return height; }

    int width;
    int height;

    // let this contain all the cells in the grid. 
    std::vector<Cell> cells; 

};

// represents a cell in the game board grid
class Cell {
public:
    bool hasBlock();
    Colour Colour();
};

struct Colour {
    int r, g, b;
};

class Block
{
public:
    Block(int x, int y, Colour col);
    ~Block();

    int X();
    int Y();
void X(int);
void Y(int);

    void Colour(const Colour& col);

private:
    int x; // x-offset from center
    int y; // y-offset from center
    Colour col; // this could be moved to the Tetroid class, if you assume that tetroids are always single-coloured
};

class Tetroid { // since you want this generalized for more than just Tetris, perhaps this is a bad name
public:
    template <typename BlockIter>
    Tetroid(BlockIter first, BlockIter last); // given a range of blocks, as represented by an iterator pair, store the blocks in the tetroid

    void Translate(int down, int left) { 
        centerX += left; 
        centerY += down;
    }
    void Rotate(int cwSteps) {
        typedef std::vector<Block>::iterator iter;
        for (iter cur = blocks.begin(); cur != blocks.end(); ++cur){
            // rotate the block (*cur) cwSteps times 90 degrees clockwise.
                    // a naive (but inefficient, especially for large rotations) solution could be this:
        // while there is clockwise rotation left to perform
        for (; cwSteps > 0; --cwSteps){
            int x = -cur->Y(); // assuming the Y axis points downwards, the new X offset is simply the old Y offset negated
            int y = cur->X(); // and the new Y offset is the old X offset unmodified
            cur->X(x);
            cur->Y(y);
        }
        // if there is any counter-clockwise rotation to perform (if cwSteps was negative)
        for (; cwSteps < 0; --cwSteps){
            int x = cur->Y();
            int y = -cur->X();
            cur->X(x);
            cur->Y(y);
        }
        }
    }

private:
    int centerX, centerY;
    std::vector<Block> blocks;
};

Tetroid Translate(Tetroid tet, int down, int left) {
    return tet.Translate(down, left);
}
Tetroid Rotate(Tetroid tet, cwSteps) {
    return tet.Rotate(cwSteps);
}

и нам нужно будет повторно внедрить проверки на спекулятивное столкновение.Учитывая немутирующие методы перевода / поворота, это просто:Мы просто создаем повернутые / переведенные копии и проверяем их на коллизию:

// test if a tetroid t would collide with the grid g if it was translated (x,y) units
if (g.Collides(Translate(t, x, y))) { ... }

// test if a tetroid t would collide with the grid g if it was rotated x times clockwise
if (g.Collides(Rotate(t, x))) { ... }

Answer 2

Я бы лично отказался от статических блоков и работал с ними как со строками.Имея статический блок, вы храните намного больше информации, чем вам нужно.

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

Миру также принадлежит один активный блок, как и у вас сейчас.Класс должен иметь метод rotate и translate .Очевидно, что блоку необходимо будет поддерживать привязку к окружающему миру, чтобы определить, столкнется ли он с существующими кирпичами или с краем доски.

Когда активный блок выйдет из игры, он вызовет что-то вроде world.update(), которое добавит фрагменты активного блока в соответствующие строки, очистит все заполненные строки, решит, проиграли ли вы и т.д., и, наконец, при необходимости создаст новый активный блок.