Text-based Tetris game with CRTP - follow-up 2

Question

Previous question:

Text-based Tetris game with CRTP - follow-up

Summary of improvements:

• Fixed issues preventing the code from compiling on Clang
• Implemented override specifier
• Fixed single letter type classes
• Elimination of magic numbers
• Removed unnecessary Block class

Summary of new features:

• Renamed clonable class to be Interfaceable
• Moved all the logic to Interfaceable class via CRTP
• Implementation of color via new cross-platform library for text-based applications

How can I improve this code further?

Tetris.cpp

#include <iostream>
#include <vector>
#include <algorithm>
#include <random>
#include <memory>
#include <chrono>

#include "rlutil.h"

using Matrix = std::vector<std::vector<int>>;

struct Point2D
{
    int x, y;
};

namespace
{
    enum Field
    {
        EMPTY = 0,
        WALL = 9
    };

    enum Menu
    {
        PLAY = 1,
        EXIT = 0
    };
};

class Shape
{
public:
    Shape() = default;

    virtual ~Shape() = default;
    virtual Shape *clone() const = 0;
    virtual int getDot(std::size_t i, std::size_t j) const = 0;
    virtual Matrix rotate() = 0;
    virtual Matrix& shape() = 0;

    std::size_t size() const
    {
        return il.size();
    }

    static const std::initializer_list<std::size_t> il;

};

const std::initializer_list<std::size_t> Shape::il =
{
    0, 1, 2, 3
};

template <typename Derived>
struct Interfaceable : public Shape
{
    virtual Shape *clone() const override
    {
        return new Derived(static_cast<const Derived&>(*this));
    }

    virtual int getDot(std::size_t i, std::size_t j) const override
    {
        return static_cast<const Derived&>(*this).shape[i][j];
    }

    virtual Matrix rotate() override
    {
        for (auto i : il)
        {
            for (auto j : il)
            {
                if (i < j)
                {
                    std::swap(static_cast<Derived&>(*this).shape[i][j], static_cast<Derived&>(*this).shape[j][i]);
                }
            }

            std::reverse(static_cast< Derived&>(*this).shape[i].begin(), static_cast<Derived&>(*this).shape[i].end());
        }

        return static_cast<Derived&>(*this).shape;
    }

    virtual Matrix& shape() override
    {
        return static_cast<Derived&>(*this).shape;
    }
};

namespace shapes
{
    class O : public Interfaceable<O>
    {
    public:
        O() = default;
        virtual ~O() = default;

        Matrix shape
        {
            {
                { 0, 0, 0, 0 },
                { 0, 1, 1, 0 },
                { 0, 1, 1, 0 },
                { 0, 0, 0, 0 }
            }
        };
    };

    class L : public Interfaceable<L>
    {
    public:
        L() = default;
        virtual ~L() = default;

        Matrix shape
        {
            {
                { 0, 0, 0, 0 },
                { 0, 1, 1, 0 },
                { 0, 0, 1, 0 },
                { 0, 0, 1, 0 }
            }
        };
    };

    class M : public Interfaceable<M>
    {
    public:
        M() = default;
        virtual ~M() = default;

        Matrix shape
        {
            {
                { 0, 1, 0, 0 },
                { 0, 1, 1, 0 },
                { 0, 0, 1, 0 },
                { 0, 0, 0, 0 }
            }
        };
    };

    class N : public Interfaceable<N>
    {
    public:
        N() = default;
        virtual ~N() = default;

        Matrix shape
        {
            {
                { 0, 0, 1, 0 },
                { 0, 1, 1, 0 },
                { 0, 1, 0, 0 },
                { 0, 0, 0, 0 }
            }
        };
    };

    class T : public Interfaceable<T>
    {
    public:
        T() = default;
        virtual ~T() = default;

        Matrix shape
        {
            {
                { 0, 0, 0, 0 },
                { 0, 1, 0, 0 },
                { 1, 1, 1, 0 },
                { 0, 0, 0, 0 }
            }
        };
    };

    class I : public Interfaceable<I>
    {
    public:
        I() = default;
        virtual ~I() = default;

        Matrix shape
        {
            {
                { 0, 1, 0, 0 },
                { 0, 1, 0, 0 },
                { 0, 1, 0, 0 },
                { 0, 1, 0, 0 }
            }
        };
    };

    class S : public Interfaceable<S>
    {
    public:
        S() = default;
        virtual ~S() = default;

        Matrix shape
        {
            {
                { 0, 0, 0, 0 },
                { 0, 1, 1, 0 },
                { 0, 1, 0, 0 },
                { 0, 1, 0, 0 }
            }
        };
    };
};

class NonCopyable
{
public:
    NonCopyable() = default;
    virtual ~NonCopyable() = default;

private:

    NonCopyable(const NonCopyable &) = delete;
    NonCopyable(const NonCopyable &&) = delete;
    NonCopyable& operator = (const NonCopyable&) = delete;
};

struct Drawable
{
    virtual void draw(std::ostream& stream) const = 0;
};

class Tetris : public Drawable, private NonCopyable
{
public:
    using Ptr = std::unique_ptr<Shape>;

    Tetris();

    void moveBlock(std::size_t, std::size_t);
    bool isCollide(std::size_t, std::size_t);
    void spawnBlock();
    bool applyRotate();
    bool isFull();

    Point2D getPosition()
    {
        return position;
    }

private:
    void initField();
    void makeBlocks();
    void checkLine();
    void makeSolid();
    Matrix mStage;

    Point2D position;

    Shape *shape;

    virtual void draw(std::ostream& stream) const;

    friend std::ostream& operator<<(std::ostream& stream, const Tetris& self)
    {
        self.draw(stream);
        return stream;
    }
    int blockType = 0;
    int mScore = 0;
    Matrix mBoard;

    std::minstd_rand rndEngine;

    std::vector<Ptr> shapes;

    static const std::initializer_list<std::size_t> ilBoard;

    static const std::initializer_list<std::size_t> ilBoardRow;
};

const std::initializer_list<std::size_t> Tetris::ilBoard =
{
    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20
};

const std::initializer_list<std::size_t> Tetris::ilBoardRow =
{
    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11
};

Tetris::Tetris()
{
    mBoard.resize(ilBoard.size(), std::vector<int>(ilBoardRow.size(), 0));
    mStage.resize(ilBoard.size(), std::vector<int>(ilBoardRow.size(), 0));

    shapes.emplace_back(std::move(std::make_unique<shapes::T>()->clone()));
    shapes.emplace_back(std::move(std::make_unique<shapes::M>()->clone()));
    shapes.emplace_back(std::move(std::make_unique<shapes::N>()->clone()));
    shapes.emplace_back(std::move(std::make_unique<shapes::I>()->clone()));
    shapes.emplace_back(std::move(std::make_unique<shapes::O>()->clone()));
    shapes.emplace_back(std::move(std::make_unique<shapes::L>()->clone()));
    shapes.emplace_back(std::move(std::make_unique<shapes::S>()->clone()));

    initField();
}

void Tetris::initField()
{
    for (auto i = ilBoard.begin(); i != ilBoard.end() - 1; ++i)
    {
        for (auto j = ilBoardRow.begin(); j != ilBoardRow.end() - 1; ++j)
        {
            if ((*j == 0) || (*j == ilBoardRow.size() - 2) || (*i == ilBoard.size() - 2))
            {
                mBoard[*i][*j] = mStage[*i][*j] = WALL;
            }
            else
            {
                mBoard[*i][*j] = mStage[*i][*j] = EMPTY;
            }
        }
    }

    makeBlocks();
}

void Tetris::makeBlocks()
{
    position.x = shape->size();
    position.y = 0;

    blockType = rndEngine() % 6;

    shape = shapes[blockType].get();

    for (auto i : shape->il)
    {
        for (auto j : shape->il)
        {
            mBoard[i][j + shape->size()] += shapes[blockType]->getDot(i, j);
        }
    }
}

bool Tetris::isFull()
{
    for (auto i : shape->il)
    {
        for (auto j : shape->il)
        {
            if (mBoard[i][j + shape->size()] > 1)
            {
                return true;
            }
        }
    }

    return false;
}
void Tetris::moveBlock(std::size_t x2, std::size_t y2)
{

    for (auto i : shape->il)
    {
        for (auto j : shape->il)
        {
            mBoard[position.y + i][position.x + j] -= shapes[blockType]->getDot(i, j);;
        }
    }

    position.x = x2;
    position.y = y2;

    for (auto i : shape->il)
    {
        for (auto j : shape->il)
        {
            mBoard[position.y + i][position.x + j] += shapes[blockType]->getDot(i, j);
        }
    }
}

void Tetris::checkLine()
{
    std::copy(mBoard.begin(), mBoard.end(), mStage.begin());

    for (auto i = ilBoard.begin() + 1; i != ilBoard.end() - 2; ++i)
    {
        bool isCompeteLine = true;

        for (auto j = ilBoardRow.begin() + 1; j != ilBoardRow.end() - 1; ++j)
        {
            if (mStage[*i][*j] == 0)
            {
                isCompeteLine = false;
            }
        }

        if (isCompeteLine)
        {
            mScore += 10;

            for (auto k : shape->il)
            {
                std::copy(mStage[*i - 1 - k].begin(), mStage[*i - 1 - k].end(), mStage[*i - k].begin());
            }
        }
    }

    std::copy(mStage.begin(), mStage.end(), mBoard.begin());
}

bool Tetris::isCollide(std::size_t x, std::size_t y)
{
    for (auto i : shape->il)
    {
        for (auto j : shape->il)
        {
            if (shapes[blockType]->getDot(i, j) && mStage[y + i][x + j] != 0)
            {
                return true;
            }
        }
    }
    return false;
}

void Tetris::makeSolid()
{
    if (isCollide(position.x, position.y + 1))
    {
        for (auto i : shape->il)
        {
            for (auto j : shape->il)
            {
                if (shapes[blockType]->getDot(i, j) != 0)
                {
                    mBoard[position.y + i][position.x + j] = WALL;
                }
            }
        }
    }
}

bool Tetris::applyRotate()
{
    Matrix temp(shape->size(), std::vector<int>(shape->size(), 0));

    std::copy(shapes[blockType]->shape().begin(), shapes[blockType]->shape().end(), temp.begin());

    shape->rotate();

    if (isCollide(position.x, position.y))
    {
        std::copy(temp.begin(), temp.end(), shapes[blockType]->shape().begin());

        return true;
    }

    for (auto i : shape->il)
    {
        for (auto j : shape->il)
        {
            mBoard[position.y + i][position.x + j] -= temp[i][j];
            mBoard[position.y + i][position.x + j] += shapes[blockType]->getDot(i, j);
        }
    }

    return false;
}

void Tetris::spawnBlock()
{
    if (!isCollide(position.x, position.y + 1))
    {
        moveBlock(position.x, position.y + 1);
    }
    else
    {
        makeSolid();
        checkLine();
        makeBlocks();
    }
}

void Tetris::draw(std::ostream& stream) const
{
    for (auto i : ilBoard)
    {
        for (auto j : ilBoardRow)
        {
            switch (mBoard[i][j])
            {
            case EMPTY:
                stream << ' ';
                break;
            case WALL:
                rlutil::setColor(3);
                stream << '@';
                break;
            case 3:
                rlutil::setColor(3);
                stream << '$';
                break;
            default:
                if (blockType == 0)
                    rlutil::setColor(rlutil::GREY);
                else
                    rlutil::setColor(blockType);
                stream << '#';
                break;
            }
        }

        stream << '\n';
    }
    rlutil::setColor(rlutil::GREY);
    stream << "Score : " << mScore
        << "\n\narrow keys left: ["
        << static_cast<char>(27) << "]\t down:["
        << static_cast<char>(25) << "]\t right:["
        << static_cast<char>(26) << "]\t Rotation:["
        << static_cast<char>(24) << "]";
}

class Game : private NonCopyable
{
public:
    int menu();
    void gameLoop();
private:
    void introScreen();
    void userInput();
    void display();
    void gameOverScreen();

    Tetris tetris;
};

void Game::gameOverScreen()
{
    gotoxy(10, 10);
    rlutil::setColor(rlutil::RED);

    std::cout << "\n"
        " #####     #    #     # ####### ####### #     # ####### ######\n"
        "#     #   # #   ##   ## #       #     # #     # #       #     #\n"
        "#        #   #  # # # # #       #     # #     # #       #     #\n"
        "#  #### #     # #  #  # #####   #     # #     # #####   ######\n"
        "#     # ####### #     # #       #     #  #   #  #       #   #\n"
        "#     # #     # #     # #       #     #   # #   #       #    #\n"
        " #####  #     # #     # ####### #######    #    ####### #     #\n"
        "\n\nPress enter to exit\n";

    std::cin.ignore();
    std::cin.get();
}

void Game::gameLoop()
{
    auto start = std::chrono::high_resolution_clock::now();

    while (!tetris.isFull())
    {
        auto end = std::chrono::high_resolution_clock::now();

        double timeTakenInSeconds = (end - start).count()
            * (static_cast<double>(std::chrono::high_resolution_clock::period::num)
            / std::chrono::high_resolution_clock::period::den);

        if (kbhit())
        {
            userInput();
        }

        if (timeTakenInSeconds > 0.3)
        {
            tetris.spawnBlock();
            display();
            start = std::chrono::high_resolution_clock::now();
        }
    }

    rlutil::cls();

    gameOverScreen();
}

int Game::menu()
{
    introScreen();

    int select_num = 0;

    std::cin >> select_num;

    switch (select_num)
    {
    case PLAY:
    case EXIT:
        break;
    default:
        select_num = 0;
        break;
    }

    return select_num;
}

void Game::introScreen()
{
    rlutil::cls();
    std::cout << "#==============================================================================#\n"
        "####### ####### ####### ######    ###    #####\n"
        "   #    #          #    #     #    #    #     #\n"
        "   #    #          #    #     #    #    #\n"
        "   #    #####      #    ######     #     #####\n"
        "   #    #          #    #   #      #          #\n"
        "   #    #          #    #    #     #    #     #\n"
        "   #    #######    #    #     #   ###    #####\t\tmade for fun \n"
        "\n\n\n\n"

        "\t<Menu>\n"
        "\t1: Start Game\n\t2: Quit\n\n"
        "#==============================================================================#\n"
        "Choose >> ";
}

void Game::display()
{
    rlutil::cls();

    std::cout << tetris;
}

void Game::userInput()
{   
    const int k = rlutil::getkey();
    switch (k)
    {
    case rlutil::KEY_RIGHT:
        if (!tetris.isCollide(tetris.getPosition().x + 1, tetris.getPosition().y))
        {
            tetris.moveBlock(tetris.getPosition().x + 1, tetris.getPosition().y);
        }
        break;
    case rlutil::KEY_LEFT:
        if (!tetris.isCollide(tetris.getPosition().x - 1, tetris.getPosition().y))
        {
            tetris.moveBlock(tetris.getPosition().x - 1, tetris.getPosition().y);
        }
        break;
    case rlutil::KEY_DOWN:
        if (!tetris.isCollide(tetris.getPosition().x, tetris.getPosition().y + 1))
        {
            tetris.moveBlock(tetris.getPosition().x, tetris.getPosition().y + 1);
        }
        break;
    case rlutil::KEY_UP:
            tetris.applyRotate();
        break;
    }
}

int main()
{
    Game game;

    switch (game.menu())
    {
    case PLAY:
        game.gameLoop();
        break;
    case EXIT:
        return 0;
    default:
        std::cerr << "Choose 1~2" << std::endl;
        return -1;
    }
}

rhutil.h

rlutil.h

Answer 1

There are a number of things here that could be improved.

Simplify your classes

There are so many classes in this code that it's hard to decipher. Most of the classes are unneeded or unhelpful, so they could and should be eliminated. One particular example is the Shape class. By itself, the name and member functions make sense. But then you have an overly complex templated Interfaceable class and then seven different classes derived from that. That's simply not warranted by the problem domain. All this code really needs is a single concrete Shape class with seven const instances.

Chose better names

In this code, there is a shapes namespace, shape member data in eight different classes, a Shape object, and a shapes member data item in one class. There are simply too many similarly (or identically!) named things to keep straight. This is a symptom that your design desperately needs simplification.

Don't allow mixin classes to be constructed

I'm not a big fan of the NonCopyable mixin class, but if you feel you must use it, make the constructor and destructor protected rather than public. Otherwise, it would allow one to create an instance of the NonCopyable class itself which would make no sense.

Eliminate useless mixin classes

The Drawable mixin class only adds visual clutter and difficulty in understanding the code and should be eliminated. Instead, simply add

Seed your random number generator

With the random number engine this code is using, minstd_rand, the seeding is not necessarily done automatically which means that the random number sequence will be the same from iteration to iteration. In the context of the game, it means that essentially the same game will be played every time. Better would be to seed the random number generator once within the constructor or better.

Use standard random distributions where practical

The code currently contains this line:

blockType = rndEngine() % 6;

The intent is apparently to get a random number in the range of 0-6, but the problem here is that the distribution is almost certainly not a uniform distribution. The only way it would be a uniform distribution would be if rndEngine()'s range is evenly divisible by seven, which is highly unlikely. Better would be to avoid these types of errors entirely by using one of the standard number distributions:

static std::uniform_int_distribution<> dist(0,6);
blockType = dist(rndEngine);

Eliminate spurious semicolons

In both of the namespaces defined in this code, there is a semicolon after the closing brace. That semicolon is not needed there and should be eliminated.

Use std::array for fixed size vectors

The size of a block is always 16, so it would make more sense to use std::array instead of a vector of vectors. It is both computationally more efficient, and also easier for other programmers to see that it's a fixed size block.

using Matrix = std::array<int, 16>;

Eliminate useless member variables

The Tetris class contains this member:

int blockType = 0;

However, that's not really ever needed. Once the local variable shape is assigned, the Tetris class shouldn't really care which kind of block it is. If you eliminate all of the derived classes and simply have a single Shape class, the block type (which you only need for coloring) should be a member of that class.

Think about the user

The first thing the program does when run is to ask the user if they want to play or quit. That doesn't make much sense. If the user ran the program, it's not hard to guess that the intent was to play the game. The menu is simply annoying and should be removed.