Recently, just for fun, I've created a clone of the widely popular game Tetris using C++. Since I am still a novice when it comes to C++, I would really appreciate all the feedback I can get from those with more experience.The code below is also on GitHub.Main.cpp#include Game.h#include <ncurses.h>#include <clocale>int main() {    Game game;    setlocale(LC_ALL, );    initscr();    start_color();    init_pair(0, COLOR_GREEN, COLOR_BLACK);    init_pair(1, COLOR_RED, COLOR_BLACK);    init_pair(2, COLOR_BLUE, COLOR_BLACK);    init_pair(3, COLOR_YELLOW, COLOR_BLACK);    init_pair(4, COLOR_GREEN, COLOR_BLACK);    curs_set(FALSE);    raw();    noecho();    nodelay(stdscr, TRUE);    game.matrix_init();    while (!game.isGameOver()) {        bool can_create_block = false;        can_create_block = game.get_last_block().move_down();        if (can_create_block) {            game.destroy();            game.create_block();        }        game.controls();        napms(game.getSpeed());        if (game.getSpeed() < DEFAULT_SPEED)            game.setSpeed(DEFAULT_SPEED);        game.draw();        game.gameOverChecker();    }    endwin();    return 0;}cCoord.h#ifndef TETRIS_CCOORD_H#define TETRIS_CCOORD_H#define MAX_COORDINATES  4class cCoord {private:    int x,        y;public:        // Getter functions    int get_x() const;    int get_y() const;    // Setter functions    cCoord set_x(int a);    cCoord set_y(int b);    cCoord(int a, int b) : x(a), y(b) {};    cCoord() = default;    ~cCoord() = default;};#endif //TETRIS_CCOORD_HcCoord.cpp#include cCoord.hint cCoord::get_x() const {    return x;}int cCoord::get_y() const {    return y;}cCoord cCoord::set_y(int b) {    y = b;    return *this;}cCoord cCoord::set_x(int a) {    x = a;    return *this;}Block.h#ifndef TETRIS_BLOCK_H#define TETRIS_BLOCK_H#include cCoord.hclass Block {private:    cCoord coord;public:    Block(cCoord c);    Block(int x, int y);    Block() = default;    ~Block() = default;    void move_down();    void move_right();    void move_left();    // Setter functions    Block set_x(int x) { coord.set_x(x); return *this; }    Block set_y(int y) { coord.set_y(y); return *this; }    // Getter functions    int get_x() const { return coord.get_x(); }    int get_y() const { return coord.get_y(); }};#endif //TETRIS_BLOCK_HBlock.cpp#include Block.h#include Game.hBlock::Block(cCoord c) : coord(c) {}Block::Block(int x, int y) : coord(x, y) {}void Block::move_down() {    coord.set_y(coord.get_y() + 1);}void Block::move_right() {    coord.set_x(coord.get_x() + 1);}void Block::move_left() {    coord.set_x(coord.get_x() - 1);}Structure.h#ifndef TETRIS_STRUCTURE_H#define TETRIS_STRUCTURE_H#include cCoord.h#include Block.h#include <vector>class Structure {    private:        int struct_type; // The type of block, according to the key        cCoord origin;        int color;    public:        Structure(int type, int c);        Structure(const Structure&);        // Rotation methods        Structure rotate_left();        Structure rotate_right();        // Movement methods        bool move_down();        Structure move_left();        Structure move_right();        std::vector<Block> coords;        // Getters        int getColor() const;};#endif //TETRIS_STRUCTURE_HStructure.cpp#include <cmath>#include Structure.h#include Game.hinline void rotate_point(cCoord &origin, float angle, Block &p) {    int x1 = static_cast<int>(round(cos(angle) * (p.get_x() - origin.get_x()) - sin(angle) * (p.get_y() - origin.get_y()) + origin.get_x()));    int y1 = static_cast<int>(round(cos(angle) * (p.get_y() - origin.get_y()) + sin(angle) * (p.get_x() - origin.get_x()) + origin.get_y()));    p.set_x(x1);    p.set_y(y1);}Structure::Structure(int type, int c) : struct_type(type), origin(Game::struct_origins[type]), color(c) {    coords.resize(4);    for (int i = 0; i < MAX_COORDINATES; ++i) {        coords.at(i).set_x(Game::struct_coords[type][i].get_x());        coords.at(i).set_y(Game::struct_coords[type][i].get_y());    }}Structure::Structure(const Structure &s) : struct_type(s.struct_type), origin(s.origin), coords(s.coords), color(s.color) {}Structure Structure::rotate_left() {    std::vector<Block> temp(coords);    // Create a temporary array to make                                        // sure the structure doesn't go out of bounds    for (auto &b : temp) {        rotate_point(origin, 1.5708, b);        // If out of bounds, do not rotate the original structure        if (b.get_x() > Game::width - 1 || b.get_x() < 0 || b.get_y() > Game::height - 1 || b.get_y() < 0 || Game::collision_detector_x(b.get_x(), b.get_y()))            return *this;    }    for (int i = 0; i < coords.size(); ++i)        coords[i] = temp[i];    return *this;}Structure Structure::rotate_right() {    std::vector<Block> temp(coords);    // Create a temporary array to make                                        // sure the structure doesn't go out of bounds    for (auto &b : temp) {        rotate_point(origin, -1.5708, b);        // If out of bounds, do not rotate the original structure        if (b.get_x() > Game::width - 1 || b.get_x() < 0 || b.get_y() > Game::height - 1 || b.get_y() < 0 || Game::collision_detector_x(b.get_x(), b.get_y()))            return *this;    }    for (int i = 0; i < coords.size(); ++i)        coords[i] = temp[i];    return *this;}bool Structure::move_down() {    for (auto &b : coords) {        if (b.get_y() >= Game::height - 1 || Game::collision_detector_y(b.get_x(), b.get_y() + 1))            return true;    }    for (auto &b : coords)        b.move_down();    if (origin.get_y() <= Game::height - 1)        origin.set_y(origin.get_y() + 1);    return false;}Structure Structure::move_left() {    std::vector<Block> temp(coords);    // Create a temporary array to make sure the                                        // structure doesn't go out of bounds    for (auto &b : temp) {        b.move_left();        // If out of bounds, do not move the original structure        if (b.get_x() > Game::width - 1 || b.get_x() < 0 || Game::collision_detector_x(b.get_x() - 1, b.get_y()))            return *this;    }    for (int i = 0; i < coords.size(); ++i)        coords[i] = temp[i];    origin.set_x(origin.get_x() - 1);    return *this;}Structure Structure::move_right() {    std::vector<Block> temp(coords);    // Create a temporary array to make sure the                                        // structure doesn't go out of bounds    for (auto &b : temp) {        b.move_right();        // If out of bounds, do not move the original structure        if (b.get_x() > Game::width - 1 || b.get_x() < 0 || Game::collision_detector_x(b.get_x() + 1, b.get_y()))        return *this;    }    for (int i = 0; i < coords.size(); ++i)        coords[i] = temp[i];    origin.set_x(origin.get_x() + 1);    return *this;}int Structure::getColor() const {    return color;}Game.h#ifndef TETRIS_GAME_H#define TETRIS_GAME_H#include Structure.h#include <vector>#include Globals.h#define DEFAULT_SPEED 300class Game {friend class Block;private:    int prev_block = 99; /* Previous block, represented by the key */    bool gameOver = false;    int speed = 250;public:    /*     * Block structures key:     * 0 : L shaped     * 1 : Square shaped     * 2 : Stick shaped     * 3 : Stair shaped     * 4 : T shaped     */    constexpr static int height = 24;    constexpr static int width = 10;    constexpr static long blockChar = L'\u2588'; /* Constant which represents the value of the block character */    static cCoord struct_coords[][MAX_COORDINATES + 1];    static cCoord struct_origins[MAX_COORDINATES + 1];    Game(); /* Constructor */    // Block/Structure functions    void create_block();    Structure& get_last_block();    void destroy(); // Destroy blocks in a line and then make all blocks ontop fall down    static bool collision_detector_y(int x, int y);    static bool collision_detector_x(int x, int y);    // Getters    int get_next_block();    bool isGameOver() const;    int getSpeed() const;    // Setters    void setSpeed(int speed);    // General game methods    void matrix_init();    void draw();    void controls();    void gameOverChecker(); // Checks for game over};#endif //TETRIS_GAME_HGame.cpp#include <random>#include <ncurses.h>#include Game.hint Game::get_next_block() {    int val;    while (true) {        std::random_device generator;        std::uniform_int_distribution<int> distribution(0,4);        if((val = distribution(generator)) != prev_block)            return val;    }}// Stores template for all the different tetris piecescCoord Game::struct_coords[][MAX_COORDINATES + 1] = {{                                                  /* Row: 1 */ {0, 0}, {1, 0}, {2, 0},                                                  /* Row: 2 */ {0, 1},                                          },                                          {                                                  /* Row: 1 */ {0, 0}, {1, 0},                                                  /* Row: 2 */ {0, 1}, {1, 1},                                          },                                          {                                                  /* Row: 1 */ {0, 0},                                                  /* Row: 2 */ {0, 1},                                                  /* Row: 3 */ {0, 2},                                                  /* Row: 4 */ {0, 3},                                          },                                          {                                                  /* Row: 1 */         {1, 0}, {2, 0},                                                  /* Row: 2 */ {0, 1}, {1, 1},                                          },                                          {                                                  /* Row: 1 */         {1, 0},                                                  /* Row: 2 */ {0, 1}, {1, 1}, {2, 1},                                          }};// Stores the origins coords for all the different tetris piecescCoord Game::struct_origins[MAX_COORDINATES + 1] = {        /* L Shaped */      {0, 0},        /* Square shaped */ {0, 0},        /* Stick shaped */  {0, 0},        /* Stair shaped */  {1, 0},        /* T shaped */      {1, 1},};Game::Game() {    create_block();}inline void Game::create_block() {    s.push_back(Structure(get_next_block(), get_next_block()));}inline Structure& Game::get_last_block() {    return *(s.end() - 1);}bool Game::isGameOver() const {    return gameOver;}void Game::matrix_init() {    int x,        y;    for (y = 0; y < height; y++) {        for (x = 0; x < width; x++) {            bool foundBlockFlag = false;            // Cycle through x and y, if x and y match with block, draw block            for (auto iter1 = s.cbegin(); iter1 != s.cend(); ++iter1)                for (auto iter2 = iter1->coords.cbegin(); iter2 != iter1->coords.cend(); ++iter2)                    if (x == iter2->get_x() && y == iter2->get_y()) {                        attron(COLOR_PAIR(iter1->getColor()));                        printw();                        attroff(COLOR_PAIR(iter1->getColor()));                        foundBlockFlag = true;                        break;                    }            // If nothing matches, draw a space            if (!foundBlockFlag) {                move(y, x);                printw( );            }        }        move(y, x);        printw(\n);    }}void Game::draw () {    for (int y = 0; y < height; y++) {        for (int x = 0; x < width; x++) {            bool foundBlockFlag = false;            // Cycle through x and y, if there is a block where there isn't a block drawn, draw one            for (auto iter1 = s.cbegin(); iter1 != s.cend(); ++iter1)                for (auto iter2 = iter1->coords.cbegin(); iter2 != iter1->coords.cend(); ++iter2)                    if (x == iter2->get_x() && y == iter2->get_y() && static_cast<char>(mvinch(y, x)) != blockChar) {                        attron(COLOR_PAIR(iter1->getColor()));                        move(y, x);                        printw();                        attroff(COLOR_PAIR(iter1->getColor()));                        foundBlockFlag = true;                        break;                    }            // If nothing matches, draw a space            if (!foundBlockFlag) {                move(y, x);                printw( );            }        }    }}void Game::controls () {    switch(getch()) {        case 'q' : case 'Q' :            get_last_block().rotate_left();            break;        case 'e' : case 'E' :            get_last_block().rotate_right();            break;        case 'a' : case 'A' :            get_last_block().move_left();            break;        case 'd' : case 'D' :            get_last_block().move_right();            break;        case 'x' : case 'X' :            gameOver = true;            break;        case 's' : case 'S' :            setSpeed(100);            break;    }}void Game::destroy() {    int counter = 0;    int delete_y;    bool fall_flag;    for (int y = height-1; y >= 1; --y) {        fall_flag = false;        for (int x = 0; x < width; ++x) {            if (mvinch(y, x) == blockChar) {                ++counter;            }            if (counter >= width) {                delete_y = y;                for (auto iter1 = s.begin(); iter1 != s.end(); ++iter1)                    for (auto iter2 = iter1->coords.begin(); iter2 != iter1->coords.end();) {                        if (iter2->get_y() == delete_y) {                            iter2 = iter1->coords.erase(iter2);                            fall_flag = true;                            continue;                        }                        ++iter2;                    }            }        }        if (fall_flag)            for (int y = delete_y - 1; y >= 0; --y) {                for (auto iter1 = s.begin(); iter1 != s.end(); ++iter1)                    for (auto iter2 = iter1->coords.begin(); iter2 != iter1->coords.end(); ++iter2) {                        if (iter2->get_y() == y)                            iter2->move_down();                    }            }        counter = 0;    }}void Game::gameOverChecker() {    if(s.size() < 2)        return;    Structure block = *(s.end() - 2);    for (auto iter1 = block.coords.cbegin(); iter1 != block.coords.cend(); ++iter1) {        if (iter1->get_y() <= 1) {            gameOver = true;            return;        }    }}int Game::getSpeed() const {    return speed;}void Game::setSpeed(int speed) {    Game::speed = speed;}bool Game::collision_detector_y(int x, int y) {    for (auto i1 = s.cbegin(); i1 != s.end() - 1; ++i1)        for (auto i2 = i1->coords.cbegin(); i2 != i1->coords.cend(); ++i2)            if (i2->get_y() == y && i2->get_x() == x)                return true;    return false;}bool Game::collision_detector_x(int x, int y) {    for (auto i1 = s.cbegin(); i1 != s.end() - 1; ++i1)        for (auto i2 = i1->coords.cbegin(); i2 != i1->coords.cend(); ++i2)            if (i2->get_x() == x && i2->get_y() == y)                return true;    return false;}Globals.h#ifndef TETRIS_GLOBALS_H#define TETRIS_GLOBALS_H#include <vector>#include Structure.h// Contains all the block structures that have fallen down, and are still fallingextern std::vector<Structure> s;#endif //TETRIS_GLOBALS_HGlobals.cpp#include Globals.h// Contains all the block structures that have fallen down, and are still fallingstd::vector<Structure> s;
Tetris clone in C++ using ncurses
c++;beginner;c++11;curses;tetris
I see a number of things that may help you improve your code.Hide implementation detailsIt was surprising to me to find so many lines of more or less raw curses code in main.  The Model-View-Controller design pattern is often useful for programs like this. The model is the internal state of the game that's mostly already within your Game class, the view is currently split between main and various parts of the Game class and the controller is essentially just the game.controls() function.  Separating the I/O from the game logic will help you write cleaner code and also assist if you were to decide to port the game to some other platform. Understand randomThe get_next_block member function of Game is currently this:int Game::get_next_block() {    int val;    while (true) {        std::random_device generator;        std::uniform_int_distribution<int> distribution(0,4);        if((val = distribution(generator)) != prev_block)            return val;    }}There are a few problems with this.  First, and most importantly, you should generally not use the std::random_device except to seed another random number generator such as mt19937.  The reason for this is that std::random_device is sometimes very slow and sometimes hardware based.  It tends to slow down a lot if the underlying entropy of the device is low.  The second problem is that the generator should probably be static so that the distribution is pulling from the same random number generator every time instead of creating a new one.   I'd rewrite it like this:int Game::get_next_block() {    static std::mt19937 generator(std::random_device{}());    std::uniform_int_distribution<int> distribution(0,4);    int val;    for (val = distribution(generator); val == prev_block; val = distribution(generator))     { }    return val;}Prefer const variables to #defineSince you're using C++, there is little reason to use #define to define a numerical constant.  Instead of this:#define DEFAULT_SPEED 300Use this:constexpr int default_speed{300};Note that I've also changed that from all capital letters (which is the convention for macros) to a regular variable name according to whichever convention you're using.Use const where practicalThere are a number of places in the code where variables could be declared const such as in Game.h:static const cCoord struct_coords[][MAX_COORDINATES + 1];static const cCoord struct_origins[MAX_COORDINATES + 1];Avoid the use of global variablesI see that s (which is a poor name, by the way) is a global variable.  It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable.  In this case, I think it would make more sense to have this be the Game object and have a separate Screen object as the Model and View classes of a Model-View-Controller.Write member initializers in declaration orderThe Structure class has this constructorStructure::Structure(const Structure &s) : struct_type(s.struct_type), origin(s.origin), coords(s.coords), color(s.color) {}That looks fine, but in fact, coords will be initialized after color because members are always initialized in declaration order and color is declared before coords in this class. To avoid misleading another programmer, you should swap the order of those such that it says instead:Structure::Structure(const Structure &s) :     struct_type(s.struct_type),     origin(s.origin),     color(s.color),    coords(s.coords){}This way the initialization actually proceeds from left to right as one might expect at first glance.Be careful with signed and unsignedIn several cases, the code compares an int i with an unsigned std::size_t coords.size().  It would be better to declare i to also be std::size_t.Pass object references where neededIt doesn't really make much sense for the collision detector functions to be static since they need the current game state to actually operate correctly.  Instead, make them regular member functions and then pass a reference to the Game object for all of the various Structure functions that call one of the collision functions.  Doing so will help you eliminate the ugly global variable as mentioned above.Rethink your classesThe cCoord class is not doing anything except cluttering the code.  This isn't Java, and the getters and setters idiom used there is not generally acceptable in modern C++.  Instead. your cCoord class could simply be a simple struct since anything can set or read it anyway.  Similarly, the Block class is also doing very little and doesn't benefit containing a cCoord -- it could be much simpler to have an x and y directly as part of the Block class.  Also, it would make more sense if the Block class actually represented a Block (with the associated coordinates and origin currently stored in Game) and handled its own rotation and reported its minimum and maximum coordinates for use in collision detection.Understand the libraries you useThe mvinch() curses function returns a chtype and not a char.  The existing code attempts to deal with that using a static cast, but that's not really correct.  Instead, I'd recommend changing the blockChar constant to be of type chtype.Avoid floating point where practicalThe current rotate_point within Structure uses sin and cos to rotate the blocks, but the blocks are only ever rotated by +90 or -90 degrees.  It would make more sense to simply precompute the sin and cos values of those angles and use those directly, which would be much faster.Think of the userThe game doesn't currently show a score, the boundaries of the playing field or any hint as to which keys do what to control the game.  Each of these would definitely enhance the user's experience.Omit return 0When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.  Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  I didn't know that. or That's bad advice!  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to it looks weird.  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered sloppy practice because it was undefined behavior (although widely supported) at the time.  So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.