Text-based Tetris game with CRTP - follow-up

Previous question:

https://codereview.stackexchange.com/questions/74677/text-based-tetris-game-follow-up-final

Summary of improvements:

• Implementation of a Drawable class

• Separate functionality, input moves to Game class

• Implementation of Cloneable class by CRTP

• Removed global variables

• Added Block class

• Improving the game loop timer by implementing `




How can I improve this code further?

Tetris.cpp

``
#include
#include
#include
#include
#include
#include

#include "utility.h"

using Matrix = std::vector>;

class Shape
{
public:
virtual ~Shape() = default;
virtual Shape *clone() const = 0;
virtual int getDrived(std::size_t i, std::size_t j) const = 0;
};

template
struct Clonable : public Shape
{
virtual Shape *clone() const
{
return new Derived(static_cast(*this));
}
};

class O : public Clonable
{
public:
O() = default;
virtual ~O() = default;

virtual int getDrived(std::size_t i, std::size_t j) const
{
return shape[i][j];
}

private:

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

class L : public Clonable
{
public:
L() = default;
virtual ~L() = default;

virtual int getDrived(std::size_t i, std::size_t j) const
{
return shape[i][j];
}

private:

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

class N : public Clonable
{
public:
N() = default;
virtual ~N() = default;

virtual int getDrived(std::size_t i, std::size_t j) const
{
return shape[i][j];
}

private:

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

class M : public Clonable
{
public:
M() = default;

Issues preventing the code from compiling on Clang:

-
In the Linux/Apple path from utility.h, function gotoxy(), you forgot a semicolon
in the last return statement:

bool gotoxy(unsigned short x = 1, unsigned short y = 1)
{
     if ((x == 0) || (y == 0))
     {
         return false;
     }

     std::cout << "\x1B[" << y << ";" << x << "H";

     return true; // <--- Was missing the ';' !
 }

-
COORD is undefined. You are relying on the definition provided by Windows.h, which obviously doesn't exist elsewhere. Either define COORD for the Unix path yourself or better, provide your own Point2D struct that replaces the non-portable Win32 COORD type.

-
std::make_unique is shamefully not available on Clang as of today. No easy fix here
but to define your own fallback replacement or avoid make_unique altogether. You can find
a code snippet for a make_unique replacement in here.

Code review:

-
When we override a function from a base class, such as clone() and getDrived() from Shape, we can now add the override specifier to aid compiler diagnostics and optimizations. Its use can also make code clearer and intentions more explicit.

-
The method name getDrived() of Shape doesn't make sense to me. What exactly is its purpose? It gets an element of the matrix that composes the char map of a shape. Then perhaps is should be called getCharacter()? But that would still sound weird, since we are talking about a shape. Then perhaps getPixel() or getDot() would be better,
if we are talking about a 2D shape that is represented by dots/pixels.

-
Speaking of the Shape derived classes, you have quite a few single letter type name representing the formats of the Tetris pieces. Since the names are single letter each, it might be nice to nest then into a namespace to give more context to the names. Maybe in a shapes namespace:

namespace shapes
{
    class O { ... };
    class L { ... };
    class M { ... };
    ...
}

Then code using them would look like this:

auto shape = make_unique();

More verbose, but with a little more context about what an O means.

-
In Tetris::draw(), it is not clear to me the meaning of the constants 0 and 9 used
in the switch statement. Zero is an empty slot, while nine seems to be the borders of the Tetris board. So those should be constants that convey this information more clearly.

-
Similar problem in Game::userInput(). The switch statement that handles the input is relying on raw numerical constants to represent the key presses. That is crying to be replaced by an enum.

-
Gameplay tip: Show some info about the controls in the home screen or before starting the game. The user can't guess which keys to press to move the pieces around.

On the architecture of Shape and derived classes:

I think you've overengineered things a bit with the whole shape hierarchy. As it is, the shape classes are just serving as holders for a matrix of chars/dots. I would either replaces the shapes by simple matrices or would make them smarter by moving the drawing and any shape-related logic to the Shape interface and subclasses. As it stands, it just adds complexity to the code. If you move more logic to the shape classes, then they would justify their existence.

Another thing, you don't have to keep those template shapes as members of Block:

class Block : private NonCopyable
{
private:

    // These guys are never used after a block is constructed!
    Ptr t;
    Ptr m;
    Ptr n;
    Ptr i;
    Ptr o;
    Ptr l;
    Ptr s;
    // All you need is this array of Shape pointers.
    std::vector shapes;
};

All you do is reference them once in the constructor and then clone each into the shapes vector. Why not place the shapes directly into the vector then and get rid of those members?

The shapes are currently just data holders, so you could make that clearer by storing pointers to const shapes, enforcing immutability:

using Ptr = std::unique_ptr;

StackExchange Code Review Q#74850, answer score: 2