5
\$\begingroup\$

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  4

class 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_H

cCoord.cpp

#include "cCoord.h"

int 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.h"

class 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_H

Block.cpp

#include "Block.h"
#include "Game.h"

Block::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_H

Structure.cpp

#include <cmath>
#include "Structure.h"
#include "Game.h"

inline 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 300

class 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_H

Game.cpp

#include <random>
#include <ncurses.h>
#include "Game.h"

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;
    }
}

// Stores template for all the different tetris pieces
cCoord 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 pieces
cCoord 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 falling
extern std::vector<Structure> s;

#endif //TETRIS_GLOBALS_H

Globals.cpp

#include "Globals.h"

// Contains all the block structures that have fallen down, and are still falling
std::vector<Structure> s;
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You need to get this reviewed. Just because I saw a file called global.cpp \$\endgroup\$ Commented Dec 17, 2016 at 18:07

1 Answer 1

Reset to default
2
\$\begingroup\$

I see a number of things that may help you improve your code.

Hide implementation details

It 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 random

The 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 #define

Since you're using C++, there is little reason to use #define to define a numerical constant. Instead of this:

#define DEFAULT_SPEED 300

Use 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 practical

There 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 variables

I 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 order

The Structure class has this constructor

Structure::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 unsigned

In 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 needed

It 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 classes

The 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 use

The 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 practical

The 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 user

The 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 0

When 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.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for the thorough and excellent review. Since C was my first programming language, followed by C++, I have kept a few bad habits. I understand that they are different languages, and should be treated as such, yet unfortunately I still make mistakes. I'm only human :). Since I'm self taught the only way I can improve is through code reviews, so you've probably just made me a better programmer. Thank you. \$\endgroup\$ Commented Dec 19, 2016 at 20:10
  • \$\begingroup\$ I have a question. I didn't really understand what you meant by quoting the MVC. I don't recall mixing the I/O and logic of the game together. If I'm wrong please don't hesitate to correct me. \$\endgroup\$ Commented Dec 19, 2016 at 20:17
  • 1
    \$\begingroup\$ The Game class includes both game logic (gameOverChecker) and I/O (draw) as well as a controller function (controls). I'm suggesting that they might be better organized in three separate classes. \$\endgroup\$ Commented Dec 20, 2016 at 13:30
  • \$\begingroup\$ That's some extremely helpful advice. Thank you. \$\endgroup\$ Commented Dec 21, 2016 at 3:39

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.