Text-based Tetris game with CRTP - follow-up

Question

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 <chrono>

How can I improve this code further?

Tetris.cpp

#include <iostream>
#include <vector>
#include <algorithm>
#include <random>
#include <memory>
#include <chrono>
#include "utility.h"
using Matrix = std::vector<std::vector<int>>;
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 <typename Derived>
struct Clonable : public Shape
{
 virtual Shape *clone() const
 {
 return new Derived(static_cast<const Derived&>(*this));
 }
};
class O : public Clonable<O>
{
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<L>
{
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<N>
{
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<M>
{
public:
 M() = default;
 virtual ~M() = default;
 virtual int getDrived(std::size_t i, std::size_t j) const
 {
 return shape[i][j];
 }
private:
 Matrix shape
 {
 {
 { 0, 0, 1, 0 },
 { 0, 1, 1, 0 },
 { 0, 1, 0, 0 },
 { 0, 0, 0, 0 }
 }
 };
};
class T : public Clonable<T>
{
public:
 T() = default;
 virtual ~T() = 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, 0, 0 },
 { 1, 1, 1, 0 },
 { 0, 0, 0, 0 }
 }
 };
};
class I : public Clonable<I>
{
public:
 I() = default;
 virtual ~I() = 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, 0, 0 },
 { 0, 1, 0, 0 },
 { 0, 1, 0, 0 }
 }
 };
};
class S : public Clonable<S>
{
public:
 S() = default;
 virtual ~S() = 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, 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 Random : private NonCopyable
{
public:
 Random(int min, int max)
 : mUniformDistribution(min, max)
 {}
 int operator()()
 {
 return mUniformDistribution(mEngine);
 }
private:
 std::default_random_engine mEngine{ std::random_device()() };
 std::uniform_int_distribution<int> mUniformDistribution;
};
class Block : private NonCopyable
{
public:
 using Ptr = std::unique_ptr<Shape>;
 Block();
protected:
 void createBlock();
 void rotateBlock();
 std::size_t size() const
 {
 return ilBlock.size();
 }
 Matrix mBlock;
 static const std::initializer_list<size_t> ilBlock;
private:
 // shapes 
 Ptr t;
 Ptr m;
 Ptr n;
 Ptr i;
 Ptr o;
 Ptr l;
 Ptr s;
 std::vector<Ptr> shapes;
 const int shapeCounter = 7;
 Random getRandom{ 0, shapeCounter - 1 };
};
const std::initializer_list<size_t> Block::ilBlock =
{
 0, 1, 2, 3
};
Block::Block()
 : t(std::make_unique<T>())
 , m(std::make_unique<M>())
 , n(std::make_unique<N>())
 , i(std::make_unique<I>())
 , o(std::make_unique<O>())
 , l(std::make_unique<L>())
 , s(std::make_unique<S>())
{
 mBlock.resize(ilBlock.size(), std::vector<int>(ilBlock.size(), 0));
 shapes.emplace_back(std::move(t->clone()));
 shapes.emplace_back(std::move(m->clone()));
 shapes.emplace_back(std::move(n->clone()));
 shapes.emplace_back(std::move(i->clone()));
 shapes.emplace_back(std::move(o->clone()));
 shapes.emplace_back(std::move(l->clone()));
 shapes.emplace_back(std::move(s->clone()));
 createBlock();
}
void Block::createBlock()
{
 int blockType = getRandom();
 for (auto i : ilBlock)
 {
 for (auto j : ilBlock)
 {
 mBlock[i][j] = shapes[blockType]->getDrived(i, j);
 }
 }
}
void Block::rotateBlock()
{
 for (auto i : ilBlock)
 {
 for (auto j : ilBlock)
 {
 if (i < j)
 {
 std::swap(mBlock[i][j], mBlock[j][i]);
 }
 }
 std::reverse(mBlock[i].begin(), mBlock[i].end());
 }
}
class Tetris : public Block, public Drawable
{
public:
 Tetris();
 void moveBlock(int, int);
 bool isCollide(int, int);
 void spawnBlock();
 bool applyRotate();
 bool isFull();
 COORD getPosition()
 {
 return position;
 }
private:
 void initField();
 void makeBlocks();
 void checkLine();
 Matrix mStage;
 COORD position;
 virtual void draw(std::ostream& stream) const;
 friend std::ostream& operator<<(std::ostream& stream, const Tetris& self)
 {
 self.draw(stream);
 return stream;
 }
 int mScore = 0;
 Matrix mBoard;
 static const std::initializer_list<size_t> ilBoard;
 static const std::initializer_list<size_t> ilBoardRow;
};
Tetris::Tetris()
{
 mBoard.resize(ilBoard.size(), std::vector<int>(ilBoardRow.size(), 0));
 mStage.resize(ilBoard.size(), std::vector<int>(ilBoardRow.size(), 0));
 initField();
}
const std::initializer_list<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<size_t> Tetris::ilBoardRow =
{
 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11
};
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] = 9;
 }
 else
 {
 mBoard[*i][*j] = mStage[*i][*j] = 0;
 }
 }
 }
 makeBlocks();
}
void Tetris::makeBlocks()
{
 position.X = ilBlock.size();
 position.Y = 0;
 createBlock();
 for (auto i : ilBlock)
 {
 for (auto j : ilBlock)
 {
 mBoard[i][j + size()] += mBlock[i][j];
 }
 }
}
bool Tetris::isFull()
{
 for (auto i : ilBlock)
 {
 for (auto j : ilBlock)
 {
 if (mBoard[i][j + size()] > 1)
 {
 return true;
 }
 }
 }
 return false;
}
void Tetris::moveBlock(int x2, int y2)
{
 for (auto i : ilBlock)
 {
 for (auto j : ilBlock)
 {
 mBoard[position.Y + i][position.X + j] -= mBlock[i][j];
 }
 }
 position.X = x2;
 position.Y = y2;
 for (auto i : ilBlock)
 {
 for (auto j : ilBlock)
 {
 mBoard[position.Y + i][position.X + j] += mBlock[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 : ilBlock)
 {
 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(int x, int y)
{
 for (auto i : ilBlock)
 {
 for (auto j : ilBlock)
 {
 if (mBlock[i][j] && mStage[y + i][x + j] != 0)
 {
 return true;
 }
 }
 }
 return false;
}
bool Tetris::applyRotate()
{
 Matrix temp(ilBlock.size(), std::vector<int>(ilBlock.size(), 0));
 std::copy(mBlock.begin(), mBlock.end(), temp.begin());
 rotateBlock();
 if (isCollide(position.X, position.Y))
 {
 std::copy(temp.begin(), temp.end(), mBlock.begin());
 return true;
 }
 for (auto i : ilBlock)
 {
 for (auto j : ilBlock)
 {
 mBoard[position.Y + i][position.X + j] -= temp[i][j];
 mBoard[position.Y + i][position.X + j] += mBlock[i][j];
 }
 }
 return false;
}
void Tetris::spawnBlock()
{
 if (!isCollide(position.X, position.Y + 1))
 {
 moveBlock(position.X, position.Y + 1);
 }
 else
 {
 checkLine();
 makeBlocks();
 }
}
void Tetris::draw(std::ostream& stream) const
{
 for (auto i : ilBoard)
 {
 for (auto j : ilBoardRow)
 {
 switch (mBoard[i][j])
 {
 case 0:
 stream << ' ';
 break;
 case 9:
 stream << '@';
 break;
 default:
 stream << '#';
 break;
 }
 }
 stream << '\n';
 }
 stream << "Score : " << mScore << "\n\n\tA: left\tS: down\tD: right \t Rotation[Space]";
}
class Game : private NonCopyable
{
public:
 int menu();
 void gameLoop();
private:
 void introScreen();
 void userInput();
 void display();
 void gameOverScreen();
 Tetris tetris;
};
void Game::gameOverScreen()
{
 std::cout << "\n"
 " ##### # # # ####### ####### # # ####### ######\n"
 "# # # # ## ## # # # # # # # #\n"
 "# # # # # # # # # # # # # # #\n"
 "# #### # # # # # ##### # # # # ##### ######\n"
 "# # ####### # # # # # # # # # #\n"
 "# # # # # # # # # # # # # #\n"
 " ##### # # # # ####### ####### # ####### # #\n"
 "\n\nPress any key and enter\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 > .3)
 {
 tetris.spawnBlock();
 display();
 start = std::chrono::high_resolution_clock::now();
 }
 }
 clearScreen();
 gameOverScreen();
}
int Game::menu()
{
 introScreen();
 int select_num = 0;
 std::cin >> select_num;
 switch (select_num)
 {
 case 1:
 case 2:
 break;
 default:
 select_num = 0;
 break;
 }
 return select_num;
}
void Game::introScreen()
{
 clearScreen();
 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()
{
 clearScreen();
 std::cout << tetris;
}
void Game::userInput()
{
 switch (_getch())
 {
 case 77:
 if (!tetris.isCollide(tetris.getPosition().X + 1, tetris.getPosition().Y))
 {
 tetris.moveBlock(tetris.getPosition().X + 1, tetris.getPosition().Y);
 }
 break;
 case 75:
 if (!tetris.isCollide(tetris.getPosition().X - 1, tetris.getPosition().Y))
 {
 tetris.moveBlock(tetris.getPosition().X - 1, tetris.getPosition().Y);
 }
 break;
 case 80:
 if (!tetris.isCollide(tetris.getPosition().X, tetris.getPosition().Y + 1))
 {
 tetris.moveBlock(tetris.getPosition().X, tetris.getPosition().Y + 1);
 }
 break;
 case 72:
 tetris.applyRotate();
 }
}
int main()
{
 Game game;
 switch (game.menu())
 {
 case 1:
 game.gameLoop();
 break;
 case 2:
 return 0;
 default:
 std::cerr << "Choose 1~2" << std::endl;
 return -1;
 }
}

utility.h

#if defined(__linux__) || defined(__APPLE__)
#include <sys/time.h>
#include <termios.h> 
#include <stdlib.h> 
#include <unistd.h> 
#include <stdio.h> 
static struct termios g_old_kbd_mode;
static void cooked(void)
{
 tcsetattr(0, TCSANOW, &g_old_kbd_mode);
}
static void raw(void)
{
 static char init;
 struct termios new_kbd_mode;
 if (init)
 {
 return;
 }
 tcgetattr(0, &g_old_kbd_mode);
 memcpy(&new_kbd_mode, &g_old_kbd_mode, sizeof(struct termios));
 new_kbd_mode.c_lflag &= ~(ICANON | ECHO);
 new_kbd_mode.c_cc[VTIME] = 0;
 new_kbd_mode.c_cc[VMIN] = 1;
 tcsetattr(0, TCSANOW, &new_kbd_mode);
 atexit(cooked);
 init = 1;
}
static int _kbhit(void)
{
 struct timeval timeout;
 fd_set read_handles;
 int status;
 raw();
 FD_ZERO(&read_handles);
 FD_SET(0, &read_handles);
 timeout.tv_sec = timeout.tv_usec = 0;
 status = select(0 + 1, &read_handles, NULL, NULL, &timeout);
 if (status < 0)
 {
 printf("select() failed in kbhit()\n");
 exit(1);
 }
 return status;
}
static int _getch(void)
{
 unsigned char temp;
 raw();
 if (read(0, &temp, 1) != 1)
 {
 return 0;
 }
 return temp;
}
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
}
void clearScreen(bool moveToStart = true)
{
 std::cout << "\x1B[2J";
 if (moveToStart)
 {
 gotoxy(1, 1);
 }
}
#elif _WIN32
#include <conio.h>
#include <Windows.h>
#include <tchar.h>
namespace
{
 HANDLE hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);
 CONSOLE_SCREEN_BUFFER_INFO csbi;
};
void clearScreen()
{
 static DWORD count;
 static DWORD cellCount;
 COORD homeCoords = { 0, 0 };
 if (!GetConsoleScreenBufferInfo(hStdOut, &csbi))
 std::cerr << "ERROR GetConsoleScreenBufferInfo - clearScreen : "
 << GetLastError() << std::endl;
 cellCount = csbi.dwSize.X *csbi.dwSize.Y;
 FillConsoleOutputCharacter(hStdOut, (TCHAR) ' ', cellCount, homeCoords, &count);
 FillConsoleOutputAttribute(hStdOut, csbi.wAttributes, cellCount, homeCoords, &count);
 SetConsoleCursorPosition(hStdOut, homeCoords);
}
#else
#error "OS not supported!"
#endif

Answer 1

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<shapes::O>();
  

  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<Ptr> 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<const Shape>;

• MORTAL: i managed to move all logic to Clonable class instead of Shape class but it becomes unplayable because of the checking for validate shape pointer that i created. here code: gist.github.com/MORTAL2000/0c9baaf4e1fcc5252939
• glampert: @MORTAL, what do you mean "unplayable"? It looks OK, though things like rotate and getDot seem misplaced on Clonable. Shouldn't those be part of Shape?
• MORTAL: i meant when you continue press key up and left for example, the pointer get invalidated. honestly i don't know why. for misplaces, i did for Shape. both of them have same result.
• MORTAL: i solved it. it needs shared_ptr rather than unique_ptr to maintain validation of pointer.