13
\$\begingroup\$

I made a Tetris game with C++ and ncurses. The game implemented the most core rules of the game such as 7-bag system or familiar rules you know about the Tetris, but lacks some features - for example, the game runs at same speed until it is over - for simplicity.

The code consists of four main classes - Block, Board, Screen, and Game class. The Block class is a representation of a block in the game, and there are derived Block classes such as IBlock, JBlock, etc. The Board class is the representation of fixed (not moving) blocks and playing board of tetris. The Screen class is a wrapper of ncurses library, which actually draws the screen of game. Lastly, the Game class runs the game with the main core logic of the Tetris.

You can move or rotate a block by hjkl - h moves the block to left, j to move down, k to rotate, l to move right.

You should install ncurses to compile this.

I want to ask if...

  1. Is this code readable? To be specific, is it possible to understand core logic of the game and design of classes for those who did not write this code?

  2. Is the naming of method/class/enum/variable in the code appropriate? Can you directly infer what the method or class does by reading the name?

  3. Is this code working well? I played for several minutes and haven't found any wierd bug yet, but is there any edge-conditions that makes game buggy?

  4. I used some global-scoped const variables to store the inital shape of the blocks, and global enum to represent the types of block. Is it the best way to write code? Or there is a better way to do things?

  5. I created several derived Block classes to represent a kind of block, which looks verbose and has some duplicated codes. Is there any better way to make derived block classes?

Makefile

tetris: main.o board.o block.o screen.o utils.o game.o
    g++ -o tetris main.o board.o block.o screen.o utils.o game.o -lncursesw

test: test.o
    g++ -o test test.o -lncursesw  
    
main.o: main.cpp board.h block.h
    g++ -c -g main.cpp
    
board.o: board.cpp board.h block.h
    g++ -c -g board.cpp

block.o: block.cpp block.h
    g++ -c -g block.cpp
    
screen.o: screen.cpp screen.h
    g++ -c -g screen.cpp -lncursesw

utils.o: utils.cpp utils.h
    g++ -c -g utils.cpp

game.o: game.cpp game.h
    g++ -c -g game.cpp

clean:
    rm -f tetris test main.o board.o block.o screen.o utils.o game.o test.o

block.h

#ifndef BLOCK_H
#define BLOCK_H

#include <vector>

struct Coordinate {
    int x, y;

    Coordinate operator+(const Coordinate &);
    Coordinate operator-(const Coordinate &);
    Coordinate operator*(int);
    Coordinate operator/(int);
};

enum BlockType {
    BLOCK_I, BLOCK_J, BLOCK_L, BLOCK_O, BLOCK_S, BLOCK_T, BLOCK_Z
};


typedef std::vector<std::vector<bool>> matrix_t;
typedef std::vector<bool> row_t;

constexpr Coordinate INITIAL_POSITION{3, 0};

static const matrix_t SHAPE_I = {{0, 0, 0, 0},
                                 {1, 1, 1, 1},
                                 {0, 0, 0, 0},
                                 {0, 0, 0, 0}};

static const matrix_t SHAPE_J = {{1, 0, 0},
                                 {1, 1, 1},
                                 {0, 0, 0}};

static const matrix_t SHAPE_L = {{0, 0, 1},
                                 {1, 1, 1},
                                 {0, 0, 0}};

static const matrix_t SHAPE_O = {{0, 1, 1, 0},
                                 {0, 1, 1, 0},
                                 {0, 0, 0, 0}};

static const matrix_t SHAPE_S = {{0, 1, 1},
                                 {1, 1, 0},
                                 {0, 0, 0}};

static const matrix_t SHAPE_T = {{0, 1, 0},
                                 {1, 1, 1},
                                 {0, 0, 0}};

static const matrix_t SHAPE_Z = {{1, 1, 0},
                                 {0, 1, 1},
                                 {0, 0, 0}};

class Block {
    private:
        Coordinate position = INITIAL_POSITION;
        Coordinate prev_position = INITIAL_POSITION;

    protected:
        Block(matrix_t initial_shape)
            : shape(initial_shape), prev_shape(initial_shape),
              row_size(initial_shape[0].size()), col_size(initial_shape.size()) {
        }

        matrix_t shape;
        matrix_t prev_shape;

        inline void save_state();

    public:
        void move_right();
        void move_left();
        void move_down();
        virtual void rotate();

        void undo();
        
        const Coordinate & get_position() const;
        const bool is_filled(const Coordinate &) const;

        const int row_size;
        const int col_size;
};

class IBlock : public Block {
    public:
        IBlock() : Block(SHAPE_I) {
        }

        void rotate() override;
};

class JBlock : public Block {
    public:
        JBlock() : Block(SHAPE_J) {
        }
};

class LBlock : public Block {
    public:
        LBlock() : Block(SHAPE_L) {
        }
};

class OBlock : public Block {
    public:
        OBlock() : Block(SHAPE_O) {
        }

        void rotate() override;
};

class SBlock : public Block {
    public:
        SBlock() : Block(SHAPE_S) {
        }
};

class TBlock : public Block {
    public:
        TBlock() : Block(SHAPE_T) {
        }
};

class ZBlock : public Block {
    public:
        ZBlock() : Block(SHAPE_Z) {
        }
};

#endif

block.cpp

#include "block.h"

Coordinate Coordinate::operator+(const Coordinate & other) {
    return Coordinate{x + other.x, y + other.y};
}

Coordinate Coordinate::operator-(const Coordinate & other) {
    return Coordinate{x - other.x, y - other.y};
}

Coordinate Coordinate::operator*(int other) {
    return Coordinate{x * other, y * other};
}

Coordinate Coordinate::operator/(int other) {
    return Coordinate{x / other, y / other};
}

inline void Block::save_state() {
    prev_position = position;
    prev_shape = shape;
    
    return;
}

void Block::move_right() {
    save_state();
    
    position.x++;
}

void Block::move_left() {
    save_state();
    
    position.x--;
}

void Block::move_down() {
    save_state();
    
    position.y++;
}

void Block::rotate() {
    save_state();

    const Coordinate origin{1, 1};

    matrix_t new_shape(col_size, row_t(row_size, false));
    Coordinate new_coordinate;

    for(int i = 0; i < col_size; i++) {
        for(int j = 0; j < row_size; j++) {
            if(shape[i][j]) {
                new_coordinate = Coordinate{j, i} - origin;
                new_coordinate = Coordinate{-new_coordinate.y, new_coordinate.x} + origin;
                new_shape[new_coordinate.y][new_coordinate.x] = true;
            }
        }
    }

    shape = new_shape;

    return;
}

//Notice that the undo() method 'undo's only one modification of the block;
//which means that, if you call undo() twice in a row,
//the block will only return to its beginning state.
void Block::undo() {
    std::swap<Coordinate>(position, prev_position);
    std::swap<matrix_t>(shape, prev_shape);

    return;
}

const Coordinate & Block::get_position() const {
    return position;
}

const bool Block::is_filled(const Coordinate & coordinate) const {
    return shape[coordinate.y][coordinate.x];
}

void IBlock::rotate() {
    save_state();

    constexpr Coordinate origin{3, 3};

    matrix_t new_shape(col_size, row_t(row_size, false));
    Coordinate new_coordinate;

    for(int i = 0; i < col_size; i++) {
        for(int j = 0; j < row_size; j++) {
            if(shape[i][j]) {
                //Because the rotational origin of the I block is in between squares,
                //I doubled up the coordinates of a square,
                //so that the newly inserted squares would represent a line between squares.
                new_coordinate = Coordinate{j, i} * 2 - origin;
                new_coordinate = Coordinate{-new_coordinate.y, new_coordinate.x} + origin;
                new_coordinate = new_coordinate / 2;
                new_shape[new_coordinate.y][new_coordinate.x] = true;
            }
        }
    }

    shape = new_shape;

    return;
}

void OBlock::rotate() {
    //The O block doesn't change its shape when the block rotates!
    save_state();

    return;
}

board.h

#ifndef BOARD_H
#define BOARD_H

#include <algorithm>
#include "block.h"

class Board {
    public:
        const int row_size = 10;
        const int col_size = 21;

    private:
        matrix_t shape;

        bool is_coordinate_out_of_bound(const Coordinate &) const;

    public:
        Board();

        int erase_full_rows();
        void fix(const Block &);

        bool is_last_row_full() const;
        bool is_valid(const Block &) const;
        bool is_filled(const Coordinate &) const;
};

#endif

board.cpp

#include "board.h"

Board::Board() : shape(col_size, row_t(row_size, false)) {
}

//Erases full rows in the board, and returns the number of erased rows.
int Board::erase_full_rows() {
    using std::all_of;
    using std::remove_if;
    using std::function;

    function<bool(row_t)> is_row_full = [](row_t row) {
        return all_of(row.begin(), row.end(), [](bool e) {return e;});
    };

    //Used erase-remove idiom to 'erase' all the full rows.
    shape.erase(remove_if(shape.begin(), shape.end(), is_row_full), shape.end());

    int erased_rows = col_size - shape.size();

    for(int i = 0; i < erased_rows; i++)
        shape.emplace(shape.begin(), row_t(row_size, false));

    return erased_rows;
}

void Board::fix(const Block & block) {
    Coordinate position = block.get_position();

    for(int i = 0; i < block.col_size; i++) {
        for(int j = 0; j < block.row_size; j++) {
            if(block.is_filled({j, i}))
                shape[position.y + i][position.x + j] = true;
        }
    }
    
    return;
}

bool Board::is_valid(const Block & block) const {
    const Coordinate & position = block.get_position();
    
    for(int i = 0; i < block.col_size; i++){
        for(int j = 0; j < block.row_size; j++){
            if(block.is_filled({j, i})) {
                if(is_coordinate_out_of_bound({position.x + j, position.y + i}))
                    return false;

                if(is_filled({position.x + j, position.y + i}))
                    return false;
            }
        }
    }
    
    return true;
}

bool Board::is_coordinate_out_of_bound(const Coordinate & coordinate) const {
    bool x_out_of_bound = (coordinate.x < 0 || coordinate.x >= row_size);
    bool y_out_of_bound = (coordinate.y < 0 || coordinate.y >= col_size);

    return x_out_of_bound || y_out_of_bound;
}

bool Board::is_filled(const Coordinate & position) const {
    return shape[position.y][position.x];
}

screen.h

#ifndef SCREEN_H
#define SCREEN_H

#include <ncursesw/ncurses.h>
#include <string>

#include "board.h"

class Screen {
    private:
        //Adding 2 to the size of the board to make a room for drawing box.
        //Multiplying 2 to the size of the row of the board
        //because the characters are half-sized.
        const int main_window_width = 2 * 10 + 2;
        const int main_window_height = 21 + 2;
        const Coordinate main_window_position{1, 1};

        const int score_window_width = 12;
        const int score_window_height = 3;
        const Coordinate score_window_position{24, 1};

        const int message_window_width = 12;
        const int message_window_height = 3;
        const Coordinate message_window_position{24, 4};

        enum ColorPair {WHITE_BLACK_PAIR, YELLOW_BLACK_PAIR};

        WINDOW * main_window;
        WINDOW * score_window;
        WINDOW * message_window;

        void draw_square_normal(const Coordinate &) const;
        void draw_square_highlighted(const Coordinate &) const;
        void erase_square(const Coordinate &) const;

        void refresh_window(WINDOW *) const;

        friend class Game;

    public:
        Screen();
        ~Screen();

        enum MessageType {
            MESSAGE_EMPTY, MESSAGE_SINGLE, MESSAGE_DOUBLE, 
            MESSAGE_TRIPLE, MESSAGE_TETRIS, MESSAGE_GAME_OVER
        };

        void update_score_window(int) const;
        void update_message_window(MessageType) const;
        void update_main_window(const Board &) const;
        void update_main_window(const Board &, const Block &) const;
};

#endif

screen.cpp

#include "screen.h"

Screen::Screen() {
    initscr();

    noecho();
    curs_set(0);

    cbreak();

    start_color();
    init_pair(WHITE_BLACK_PAIR, COLOR_WHITE, COLOR_BLACK);
    init_pair(YELLOW_BLACK_PAIR, COLOR_YELLOW, COLOR_BLACK);

    main_window = newwin(main_window_height,
                         main_window_width,
                         main_window_position.y,
                         main_window_position.x);

    score_window = newwin(score_window_height,
                          score_window_width,
                          score_window_position.y,
                          score_window_position.x);
    
    message_window = newwin(message_window_height,
                            message_window_width,
                            message_window_position.y,
                            message_window_position.x);

    wtimeout(main_window, 0);

    box(main_window, 0, 0);
    box(score_window, 0, 0);
    box(message_window, 0, 0);

    mvwprintw(main_window, 0, 1, "Game");
    mvwprintw(score_window, 0, 1, "Score");
    mvwprintw(message_window, 0, 1, "Message");
}

Screen::~Screen() {
    delwin(main_window);
    endwin();
}

void Screen::update_main_window(const Board & board, const Block & block) const {
    for(int i = 0; i < board.col_size; i++) {
        for(int j = 0; j < board.row_size; j++) {
            if(board.is_filled({j, i}))
                draw_square_normal({j, i});
            else
                erase_square({j, i});
        }
    }

    Coordinate position = block.get_position();

    //Because previously drawn squares of board are already existing,
    //you must not erase any empty squares when drawing block.
    for(int i = 0; i < block.col_size; i++) {
        for(int j = 0; j < block.row_size; j++) {
            if(block.is_filled({j, i}))
                draw_square_highlighted({position.x + j, position.y + i});
        }
    }

    refresh_window(main_window);

    return;
}

void Screen::update_main_window(const Board & board) const {
    for(int i = 0; i < board.col_size; i++) {
        for(int j = 0; j < board.row_size; j++) {
            if(board.is_filled({j, i}))
                draw_square_normal({j, i});
            else
                erase_square({j, i});
        }
    }

    refresh_window(main_window);
}

void Screen::draw_square_normal(const Coordinate & coordinate) const {
    Coordinate cursor_coordinate{2 * coordinate.x + 1, coordinate.y + 1};

    //The space character is half-sized,
    //so the game would be looking so weird if you draw a square with just a single mvwaddch() call.
    mvwaddch(main_window, cursor_coordinate.y, cursor_coordinate.x, 
            ' ' | A_REVERSE | COLOR_PAIR(WHITE_BLACK_PAIR));
    mvwaddch(main_window, cursor_coordinate.y, cursor_coordinate.x + 1, 
            ' ' | A_REVERSE | COLOR_PAIR(WHITE_BLACK_PAIR));

    return;
}

void Screen::draw_square_highlighted(const Coordinate & coordinate) const {
    Coordinate cursor_coordinate{2 * coordinate.x + 1, coordinate.y + 1};

    mvwaddch(main_window, cursor_coordinate.y, cursor_coordinate.x, 
            ' ' | A_REVERSE | COLOR_PAIR(YELLOW_BLACK_PAIR));
    mvwaddch(main_window, cursor_coordinate.y, cursor_coordinate.x + 1, 
            ' ' | A_REVERSE | COLOR_PAIR(YELLOW_BLACK_PAIR));

    return;
}

void Screen::erase_square(const Coordinate & coordinate) const {
    Coordinate cursor_coordinate{2 * coordinate.x + 1, coordinate.y + 1};

    mvwaddch(main_window, cursor_coordinate.y, cursor_coordinate.x, 
            ' ' | COLOR_PAIR(WHITE_BLACK_PAIR));
    mvwaddch(main_window, cursor_coordinate.y, cursor_coordinate.x + 1, 
            ' ' | COLOR_PAIR(WHITE_BLACK_PAIR));

    return;
}

void Screen::update_score_window(int score) const {
    Coordinate cursor_coordinate{1, 1};

    mvwprintw(score_window, cursor_coordinate.y, cursor_coordinate.x, "%d", score);
    refresh_window(score_window);

    return;
}

void Screen::update_message_window(MessageType message) const {
    using std::string;

    Coordinate cursor_coordinate{1, 1};
    string message_str;

    switch(message) {
        case MESSAGE_EMPTY:
            message_str = "";
            break;
        case MESSAGE_SINGLE:
            message_str = "Single!";
            break;
        case MESSAGE_DOUBLE:
            message_str = "Double!";
            break;
        case MESSAGE_TRIPLE:
            message_str = "Triple!";
            break;
        case MESSAGE_TETRIS:
            message_str = "Tetris!";
            break;
        case MESSAGE_GAME_OVER:
            message_str = "Game Over!";
            break;
    }

    mvwprintw(message_window,
              cursor_coordinate.y,
              cursor_coordinate.x,
              "%s",
              message_str.c_str());

    refresh_window(message_window);

    return;
}

void Screen::refresh_window(WINDOW * window) const {
    wrefresh(window);
    
    return;
}

game.h

#ifndef GAME_H
#define GAME_H

#include <map>
#include <memory>
#include <algorithm>
#include <chrono>
#include <thread>

#include "block.h"
#include "board.h"
#include "screen.h"
#include "utils.h"

typedef std::unique_ptr<Block> block_ptr_t;

class Game {
    private:
        bool is_game_end;
        CyclicCounter force_down_counter;
        RandomGenerator random_generator;

        Screen game_screen;
        Board game_board;
        block_ptr_t current_block;
        unsigned int score = 0;

        void run_single_frame();
        block_ptr_t generate_block();
        void sleep_for_frame_duration() const;
        void sleep_for_user_input() const;
        const char get_user_input() const;

        const unsigned int score_per_erased_row = 100;
        const unsigned int score_per_tetris = 1000;

    public:
        Game();

        void run();
};

#endif

game.cpp

#include "game.h"

Game::Game() : force_down_counter(), 
               game_screen(),
               game_board(), 
               current_block(generate_block()),
               random_generator(BLOCK_I, BLOCK_Z) {
    game_screen.update_score_window(score);
    game_screen.update_message_window(Screen::MESSAGE_EMPTY);
}

void Game::run() {
    do {
        run_single_frame();
    } while(!is_game_end);

    game_screen.update_message_window(Screen::MESSAGE_GAME_OVER);
    sleep_for_user_input();

    return;
}

void Game::run_single_frame() {
    sleep_for_frame_duration();

    if(!current_block) {
        current_block = generate_block();

        if(!game_board.is_valid(*current_block)) {
            is_game_end = true;
            return;
        }
    }

    bool is_block_bottom_touched = false;

    if(force_down_counter.is_zero()) {
        current_block->move_down();

        if(game_board.is_valid(*current_block)) {
            game_screen.update_main_window(game_board, *current_block);
        } else {
            current_block->undo();
            is_block_bottom_touched = true;
        }
    }

    force_down_counter--;

    char user_input = get_user_input();
    switch(user_input) {
        case 'h':
            current_block->move_left();
            break;
        case 'j':
            current_block->move_down();

            if(!game_board.is_valid(*current_block)) {
                current_block->undo();
                is_block_bottom_touched = true;
            }

            break;
        case 'k':
            current_block->rotate();
            break;
        case 'l':
            current_block->move_right();
            break;
    }

    if(game_board.is_valid(*current_block))
        game_screen.update_main_window(game_board, *current_block);
    else
        current_block->undo();

    unsigned int erased_rows;

    if(is_block_bottom_touched) {
        game_board.fix(*current_block);
        current_block.reset();

        erased_rows = game_board.erase_full_rows();

        if(erased_rows == 4) {
            score += score_per_tetris;
            game_screen.update_message_window(Screen::MESSAGE_TETRIS);
        } else if(erased_rows > 0) {
            //The value MESSAGE_SINGLE, MESSAGE_DOUBLE, and MESSAGE_TRIPLE has initial value of 1, 2, 3...
            //So, the static_cast of integer to MessageType converts the number of erased rows to the message.
            score += erased_rows * score_per_erased_row;
            game_screen.update_message_window(static_cast<Screen::MessageType>(erased_rows));
        } else {
            game_screen.update_message_window(Screen::MESSAGE_EMPTY);
        }

        game_screen.update_main_window(game_board);
        game_screen.update_score_window(score);

        return;
    }

    return;
}

void Game::sleep_for_frame_duration() const {
    static const std::chrono::milliseconds frame_duration_in_ms(100);
    std::this_thread::sleep_for(frame_duration_in_ms);

    return;
}

block_ptr_t Game::generate_block() {
    using std::map;
    using std::pair;
    using std::all_of;

    static map<BlockType, bool> is_generated;

    bool is_every_type_generated 
        = all_of(is_generated.begin(),
                 is_generated.end(),
                 [](pair<BlockType, bool> e) {return e.second;});

    if(is_every_type_generated) {
        is_generated.clear();

        for(int i = BLOCK_I; i != BLOCK_Z; i++)
            is_generated[static_cast<BlockType>(i)] = false;
    }

    BlockType random_block_type;

    do {
        random_block_type
            = static_cast<BlockType>(random_generator());
    } while(is_generated[random_block_type]);

    is_generated[random_block_type] = true;

    switch(random_block_type) {
        case BLOCK_I: return block_ptr_t(new IBlock()); 
        case BLOCK_J: return block_ptr_t(new JBlock()); 
        case BLOCK_L: return block_ptr_t(new LBlock()); 
        case BLOCK_O: return block_ptr_t(new OBlock()); 
        case BLOCK_S: return block_ptr_t(new SBlock()); 
        case BLOCK_T: return block_ptr_t(new TBlock()); 
        case BLOCK_Z: return block_ptr_t(new ZBlock()); 
        default: return nullptr;
    }
}

void Game::sleep_for_user_input() const {
    wtimeout(game_screen.main_window, -1);
    wgetch(game_screen.main_window);
    wtimeout(game_screen.main_window, 0);

    return;
}

const char Game::get_user_input() const {
    char user_input = wgetch(game_screen.main_window);
    flushinp();
    return user_input;
}

utils.h

#ifndef UTILS_H
#define UTILS_H

#include <random>

class CyclicCounter {
    private:
        const int initial_value;
        int value;

    public:
        CyclicCounter(int initial_value = 5);

        bool is_zero() const;
        CyclicCounter operator--(int);
};

class RandomGenerator {
    private:
        std::random_device device;
        std::minstd_rand generator;
        std::uniform_int_distribution<int> distribution;

    public:
        RandomGenerator(int, int);

        int operator()();
};

#endif

utils.cpp

#include "utils.h"

CyclicCounter::CyclicCounter(int initial_value) : initial_value(initial_value),
                                                      value(initial_value) {
}

bool CyclicCounter::is_zero() const {
    return value == 0;
}

CyclicCounter CyclicCounter::operator--(int) {
    CyclicCounter original = *this;

    if(is_zero())
        value = initial_value;
    else
        value--;

    return original;
}

//Constructs random number generator that produces random number in range of [first, last].      
RandomGenerator::RandomGenerator(int first, int last) : device(),
                                                        generator(device()),                     
                                                        distribution(first, last) {
}

int RandomGenerator::operator()() {
    return distribution(generator);
}

main.cpp

#include "game.h"

int main(void){
    Game game;
    game.run();

    return 0;
}
\$\endgroup\$
4
  • \$\begingroup\$ Not downvoting, but I feel this is too long of a program to review here on the site. Thus, for example, you're getting an answer limited to your Makefile only... \$\endgroup\$
    – einpoklum
    Sep 16, 2023 at 9:09
  • 2
    \$\begingroup\$ @einpoklum If it fits it is suitable for this site. It is better if we are able to review the whole program, rather than just snippets without context. It's also fine if the answers are just reviewing part of the code, more answers can cover the rest. \$\endgroup\$
    – G. Sliepen
    Sep 16, 2023 at 10:28
  • \$\begingroup\$ @einpoklum, on the contrary, it would be easier to to review if test.cpp was also provided. \$\endgroup\$ Sep 17, 2023 at 10:22
  • \$\begingroup\$ Why is SHAPE_O not a 2x2 matrix? \$\endgroup\$
    – web4141
    Dec 23, 2023 at 19:38

3 Answers 3

12
\$\begingroup\$

Makefile review

Most of the rules in the Makefile aren't needed. Make comes with some rules built-in (which you can see with make --print-data-base -f /dev/null):

%.o: %.cpp
#  recipe to execute (built-in):
  $(COMPILE.cpp) $(OUTPUT_OPTION) $<

$(COMPILE.cpp) expands to $(CXX) $(CXXFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c, so we can just let that rule do all the work for us.

That reduces the *.o rules to just specifying the header dependencies:

main.o: main.cpp board.h block.h
board.o: board.cpp board.h block.h
block.o: block.cpp block.h
screen.o: screen.cpp screen.h
utils.o: utils.cpp utils.h
game.o: game.cpp game.h

We can get the compiler to generate those dependencies for us, instead of having to maintain them by hand:

CXXFLAGS += -MMD
-include $(wildcard *.d)

We can add our usual warning options, too. Here's a fragment from one of my own makefiles:

WARNINGS += -Wall -Wextra -Wpedantic -Weffc++
WARNINGS += -Wwrite-strings -Wno-parentheses -Warray-bounds
WARNINGS += -Wold-style-cast -Wuseless-cast
WARNINGS += -Wdisabled-optimization -Wstrict-overflow=4 -Wno-error=strict-overflow
WARNINGS += -Woverloaded-virtual
WARNINGS += -Winit-self -Wpointer-arith
WARNINGS += -Wlogical-op
WARNINGS += -Wfloat-equal
WARNINGS += -Wconversion
WARNINGS += -Wshadow

CXXFLAGS += -std=c++20
CXXFLAGS += -Werror
CXXFLAGS += $(WARNINGS)
CXXFLAGS += -g -O3

We can simplify linking the test program too, using our knowledge of the built-in rule:

%: %.o
#  recipe to execute (built-in):
  $(LINK.o) $^ $(LOADLIBES) $(LDLIBS) -o $@

We can set LINK.o = $(LINK.cpp) to use the correct linker for C++, and add the library to LDLIBS, then the default rule will do the job:

LINK.o = $(LINK.cpp)
LDLIBS += -lncursesw

We can get the library list more accurately using pkg-config:

LDLIBS += $(shell pkg-config --libs ncursesw)

If I have a lot of package dependencies, I tend to put them in a variable, so we can use pkg-config consistently for compiling and linking:

PKGS = ncursesw
CXXFLAGS += $(shell pkg-config --cflags $(PKGS))
LDLIBS += $(shell pkg-config --lflags $(PKGS))

To use default rules for the tetris binary, we just need to rename main.cpp to tetris.cpp.

Assuming test.cpp contains unit-tests, we should make the build fail if there are any failing tests:

tetris: run-tests

run-tests: test
    ./$<

.PHONY: run-tests

(As an aside, I don't recommend calling your test program test - that often trips people up when they accidentally run a shell built-in or system program - use something distinctive such as tetris-test.)

Finally, we need to add clean to the list of phony targets, and like all makefiles, add a rule for .DELETE_ON_ERROR so that a failed command doesn't leave behind a partial output file that prevents rebuilding.


Putting this all together, the resultant makefile looks something like this:

LINK.o = $(LINK.cpp)

PKGS = ncursesw

CXXFLAGS += $(shell pkg-config --cflags $(PKGS))
LDLIBS += $(shell pkg-config --libs $(PKGS))

WARNINGS += -Wall -Wextra -Wpedantic -Weffc++
WARNINGS += -Wwrite-strings -Wno-parentheses -Warray-bounds
WARNINGS += -Wold-style-cast -Wuseless-cast
WARNINGS += -Wdisabled-optimization -Wstrict-overflow=4 -Wno-error=strict-overflow
WARNINGS += -Woverloaded-virtual
WARNINGS += -Winit-self -Wpointer-arith
WARNINGS += -Wlogical-op
WARNINGS += -Wfloat-equal
WARNINGS += -Wconversion
WARNINGS += -Wshadow

CXXFLAGS += -std=c++20
CXXFLAGS += -Werror
CXXFLAGS += $(WARNINGS)
CXXFLAGS += -g -O3

CXXFLAGS += -MMD
-include $(wildcard *.d)

# first target is the default
tetris: tetris.o board.o block.o screen.o utils.o game.o
tetris.o: run-tests

run-tests: test
    ./$<

clean:
    $(RM) tetris test *.o

.DELETE_ON_ERROR:
.PHONY: clean run-tests
\$\endgroup\$
4
  • 2
    \$\begingroup\$ You technically don’t need .DELETE_ON_ERROR, because all C++ tools delete on error anyway. Doesn’t hurt, though (unless you care about POSIX compatibility, which I presume not, since you’re using other GNU-isms). Instead of CXX ::= g++, you could do CXX ?= g++, which will set CXX to g++ iff CXX is not already defined. ?= is POSIX (or will be). But I don’t recommend setting CXX or flags; let the user do that. I think there is an error in your pkg-config call for LDLIBS; pretty sure you want --libs, not --lflags Finally, it’s probably not wise to name an exe test. \$\endgroup\$
    – indi
    Sep 17, 2023 at 0:00
  • \$\begingroup\$ Thanks @indi for those corrections. I'll keep .DELETE_ON_ERROR as it's a good habit and does no harm. I did once run into trouble with a partially-written file when I ran out of storage during write, but can't remember whether that was using a redirected shell command or a compiler (and if the latter, whether it was a GNU one - but it certainly wasn't a recent one). I always like to get that in at the start, because makefiles tend to grow and eventually include a command that doesn't remove its outputs when it fails! \$\endgroup\$ Sep 17, 2023 at 7:50
  • \$\begingroup\$ I follow the standard advice these days to always use GNU make (that said, an unused target is perfectly fine as far as other implementations are concerned, so .DELETE_ON_ERROR: is benign even where it's not recognised as magic). \$\endgroup\$ Sep 17, 2023 at 7:58
  • \$\begingroup\$ Thank you for reviewing Makefile! I writed all of the targets and objects because I just know very basics of Makefile. I'll improve my Makefile by adding suggestions of your answer. \$\endgroup\$ Sep 18, 2023 at 6:28
4
\$\begingroup\$

Representing shapes

You defined a std::vector<std::vector<bool>> matrix_t, and use that to hold shapes. Vectors of vectors are quite inefficient though. You can do a lot better if you use a fixed 4x4 size to store all shapes. You can then use std::array and/or std::bitset. For example:

using Shape = std::bitset<16>;

static const Shape SHAPE_I("0000"
                           "1111");

static const Shape SHAPE_J("1000"
                           "1110");
…

Use of inheritance for block shapes

Using inheritance to make separate classes for each shape is one way to do it, and is not wrong per se, but it has some issues. I would suggest a different approach: have a concrete Block class that does everything. Rotating a 4x4 shape can be done using a single function that works for all shapes. The only issue might be to adjust the position afterwards, depending on where the "center" of each shape is. Either you could make some algorithm that deduces this from the bits that are set, or you could simply store some extra information in Block to help do that. So for example:

class Block {
    …
    Block(Shape initial_shape, Coordinate origin, …): … {}
    void rotate();
    …
};

One advantage of not using inheritance is that you don't have to use pointers anymore. So for example, you could write:

static const Block blocks[] = {
    {SHAPE_J, …},
    …
    {SHAPE_Z, …},
};

Block Game::generate_block() {
    …
    return blocks[random_block_type];
}

Separate the user interface from the game logic better

I think you already did a very good job at organizing the code into separate files and classes. You have Screen for the output to the screen, and Block and Board to handle most of the game state itself. You tie this together in Game. The only issue I see is that too much is still being done in Game's member functions. For example, it uses ncurses directly to get keyboard input. Also run_single_frame() does a lot of things that might better be split off in separate member functions.

If Board and Blocks are just used to track the game's state, then Game is the place to implement the game's logic. However, input and output should then be done outside that class, and Game should only provide member functions like left(), right(), rotate() and down() to signal input actions, and updating the screen should also be done by an outside function that just queries Game what the state is of the board and the current block.

Ideally, you should be able to replace the ncurses interface with, say, an SDL interface, without having to change anything in Block, Board and Game. You probably will then naturally implement the model-view-controller design pattern.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Great! Using the std::bitset would help program run a lot faster, and I agree that there is no need to use vector since the program only stores a bool values to represent a block. However, the idea of making just a single block class and having it to store the shape and the origin of blocks is somewhat hard to apply because the logic of Block::rotate() is completely different for IBlock and OBlock. I think letting Block class store an BlockType variable and making the Block::rotate act differently depending on the variable can be one solution. Any better idea of solving this problem? \$\endgroup\$ Sep 18, 2023 at 6:43
  • \$\begingroup\$ You could have a std::function member variable in Block that points to the desired rotation function. \$\endgroup\$
    – G. Sliepen
    Sep 18, 2023 at 7:54
2
\$\begingroup\$

Picking out one function that attracted my attention:

int Board::erase_full_rows() {
    using std::all_of;
    using std::remove_if;
    using std::function;

    function<bool(row_t)> is_row_full = [](row_t row) {
        return all_of(row.begin(), row.end(), [](bool e) {return e;});
    };

    //Used erase-remove idiom to 'erase' all the full rows.
    shape.erase(remove_if(shape.begin(), shape.end(), is_row_full), shape.end());

    int erased_rows = col_size - shape.size();

    for(int i = 0; i < erased_rows; i++)
        shape.emplace(shape.begin(), row_t(row_size, false));

    return erased_rows;
}

I always say it's good practice to localise using statements like that, so an initial thumbs-up there.

Let's look at where those identifiers are used. all_of() and remove_if() are called with vector iterators. Normally a standard library iterator would be in namespace std, so argument-dependent lookup would find the function name without qualification. However, std::vector is unusual in that its iterators may be bare pointers, preventing this ADL. We should consider the std::ranges versions of these functions, though, for their improved interface.

The other identifier we're using is std::function. But we don't really need the type erasure that this provides, so we could use the lambda directly:

    static auto const is_row_full = [](row_t row) {
        return all_of(row.begin(), row.end(), [](bool e) {return e;});
    };

That also neatly side-steps our failure to include <functional>. ;-)

That gets us to:

//Erases full rows in the board, and returns the number of erased rows.
int Board::erase_full_rows() {

    static auto const is_row_full = [](row_t row) {
        return std::ranges::all_of(row, [](bool e) {return e;});
    };

    //Used erase-remove idiom to 'erase' all the full rows.
    shape.erase(std::ranges::remove_if(shape, is_row_full).end(), shape.end());

    int erased_rows = col_size - shape.size();

    for(int i = 0; i < erased_rows; i++)
        shape.emplace(shape.begin(), row_t(row_size, false));

    return erased_rows;
}

My next observation is that it seems inefficient to destroy the removed rows only to insert new rows at the front of the vector (remember, that's the very worst place to add to a vector). We can do better by using a reverse-view of the board, so that the retained rows move to the bottom and then we have indeterminate vectors at the top ready to be assigned as empty rows. That would give us something like this:

//Erases full rows in the board, and returns the number of erased rows.
std::size_t Board::erase_full_rows() {

    // Move the full rows to the end
    static auto const is_row_full = [](const row_t& row) {
        return std::ranges::all_of(row, [](bool e) {return e;});
    };
    auto full_rows = std::ranges::remove_if(shape | std::views::reverse, is_row_full);

    // empty them all
    auto const empty_row = row_t(row_size, false);
    fill(full_rows, empty_row);

    return full_rows.size();
}

Or, with a well-scoped using directive:

// Clear full rows in the board
// @return the number of cleared rows.
std::size_t Board::erase_full_rows()
{
    using namespace std::ranges;

    // Remove the full rows (but don't delete them)
    auto const full_row = row_t(row_size, true);
    auto full_rows = remove(shape | views::reverse, full_row);

    // Empty the removed rows
    auto const empty_row = row_t(row_size, false);
    fill(full_rows, empty_row);

    return full_rows.size();
}

Unfortunately, we still have to reconstruct the rows, as the moved-from state may be a different size to the populated vectors. We could use std::ranges::stable_partition() instead of remove_if() to preserve the full rows and avoid vector reallocation. We'll need a slight tweak to the predicate, since stable_partition() returns the range for which the predicate is false, unlike remove_if():

// Clear full rows in the board
// @return the number of cleared rows.
std::size_t Board::erase_full_rows()
{
    using namespace std::ranges;

    // Move the full rows to the top
    static auto const row_not_full = [](const row_t& row) {
        return any_of(row, [](bool e) {return !e;});
    };
    auto full_rows = stable_partition(shape | views::reverse, row_not_full);

    // Empty the full rows
    static auto const clear_row = [](row_t& row){
        fill(row, false);
    };
    for_each(full_rows, clear_row);

    return full_rows.size();
}

We could of course do this the other way around (stable_partition(shape, row_is_full)), but in this case the complication is to form the full_rows subrange from shape.begin() and stable_partition(…).begin():

    static auto const row_is_full = [](const row_t& row) {
        return all_of(row, [](bool e) {return e;});
    };
    auto full_rows = subrange(shape.begin(),
                              stable_partition(shape, row_is_full).begin());

Not much to choose between these last two, really.

\$\endgroup\$
1
  • \$\begingroup\$ This answer assumes that you'll add CXXFLAGS += -std=c++20 (for GNU Make and GCC) and add includes for relevant headers <algorithm> and <ranges>. \$\endgroup\$ Sep 18, 2023 at 8:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.