2
\$\begingroup\$

I just finished writing an ncurses-based Tetris clone in C. It's only my second project of this size, the first being a Snake clone. I would really appreciate any and all suggestions/improvements as I'm relatively new to C and I would like to improve.

As this is a 300-plus-line codebase, anything I can add to this question to more easily facilitate its review, I will.

main.c:

#include "game.h"
#include "ncurses_util.h"

#include <ncurses.h>
#include <unistd.h>

int main(void) {
    ncurses_init();
    curs_set(0); // Invisible cursor
    draw_game_board();

    int ch;
    enum Direction dir;
    while ((ch = getch())) {
        switch (ch) {
        case KEY_DOWN: dir = Down; break;
        case KEY_UP: dir = Up; break;
        case KEY_LEFT: dir = Left; break;
        case KEY_RIGHT: dir = Right; break;
        default: dir = None; break;
        }
        update_game(dir);
        usleep(20000); // 50FPS
    }
}

game.h:

#ifndef TETRIS_GAME_H
#define TETRIS_GAME_H

#define BOARD_WIDTH  10
#define BOARD_HEIGHT 12

enum Direction { Up, Down, Left, Right, None };

struct Coords {
    int y;
    int x;
};

void draw_game_board();
void update_game(enum Direction press);

#endif /* TETRIS_GAME_H */

game.c:

#include "game.h"

#include "ncurses_util.h"

#include <ncurses.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>

enum Piece { T, S, Z, L, J, O, I };

const struct Coords pieces[7][4] = {
    { { 1, 4 }, { 2, 5 }, { 1, 6 }, { 1, 5 } }, // T
    { { 2, 5 }, { 1, 6 }, { 2, 4 }, { 1, 5 } }, // S
    { { 1, 4 }, { 2, 6 }, { 2, 5 }, { 1, 5 } }, // Z
    { { 1, 6 }, { 2, 4 }, { 2, 6 }, { 2, 5 } }, // L
    { { 1, 4 }, { 2, 4 }, { 2, 6 }, { 2, 5 } }, // J
    { { 1, 5 }, { 1, 6 }, { 2, 5 }, { 2, 6 } }, // O
    { { 1, 4 }, { 1, 5 }, { 1, 6 }, { 1, 7 } }, // I
};

int tetris_check() {
    static char *fullLine;
    static char *emptyLine;
    if (!fullLine) {
        fullLine = malloc(sizeof(char) * (BOARD_WIDTH + 1));
        emptyLine = malloc(sizeof(char) * (BOARD_WIDTH + 1));
        for (int i = 0; i < BOARD_WIDTH; i++) {
            fullLine[i] = '#';
            emptyLine[i] = ' ';
        }
        fullLine[BOARD_WIDTH] = '\0';
        emptyLine[BOARD_WIDTH] = '\0';
    }

    int tetrises = 0;
    char *line = malloc(sizeof(char) * (BOARD_WIDTH + 1));
    for (int i = 1; i <= BOARD_HEIGHT; i++) {
        mvinnstr(i, 1, line, BOARD_WIDTH);
        if (strcmp(line, fullLine) == 0) {
            mvaddstr(i, 1, emptyLine);
            tetrises++;
        }
    }

    int dropAmount = 0;
    for (int i = BOARD_HEIGHT + 1; i >= 1; i--) {
        mvinnstr(i, 1, line, BOARD_WIDTH);
        if (strcmp(line, emptyLine) == 0) {
            dropAmount++;
        } else {
            mvaddstr(i + dropAmount, 1, line);
            if (dropAmount) {
                mvaddstr(i, 1, emptyLine);
            }
        }
    }
    return tetrises;
}

void rotate_piece(struct Coords *originalPiece, enum Piece pieceType) {
    if (pieceType == O) return; // No point in rotating an O
    struct Coords piece[4];
    memcpy(piece, originalPiece, sizeof(struct Coords) * 4);
    if (pieceType == I) {
        if (piece[0].x == piece[1].x) { // Horizontal
            for (int i = 0; i < 4; i++) {
                if (i == 1) continue;
                piece[i].x = piece[1].x + (piece[i].y - piece[1].y);
                piece[i].y = piece[1].y;
            }
        } else { // Vertical
            for (int i = 0; i < 4; i++) {
                if (i == 1) continue;
                piece[i].y = piece[1].y + (piece[i].x - piece[1].x);
                piece[i].x = piece[1].x;
            }
        }
    } else {
        // The last coordinate pair always corresponds to the "center" of a non-I/O piece
        for (int i = 0; i < 3; i++) {
            if (piece[i].x == piece[3].x && piece[i].y == piece[3].y - 1) {
                piece[i].x += 1;
                piece[i].y += 1;
            } else if (piece[i].x == piece[3].x && piece[i].y == piece[3].y + 1) {
                piece[i].x -= 1;
                piece[i].y -= 1;
            } else if (piece[i].x == piece[3].x - 1 && piece[i].y == piece[3].y) {
                piece[i].x += 1;
                piece[i].y -= 1;
            } else if (piece[i].x == piece[3].x + 1 && piece[i].y == piece[3].y) {
                piece[i].x -= 1;
                piece[i].y += 1;
            } else if (piece[i].x == piece[3].x - 1 && piece[i].y == piece[3].y - 1) {
                piece[i].x += 2;
            } else if (piece[i].x == piece[3].x + 1 && piece[i].y == piece[3].y - 1) {
                piece[i].y += 2;
            } else if (piece[i].x == piece[3].x + 1 && piece[i].y == piece[3].y + 1) {
                piece[i].x -= 2;
            } else if (piece[i].x == piece[3].x - 1 && piece[i].y == piece[3].y + 1) {
                piece[i].y -= 2;
            }
        }
    }
    for (int i = 0; i < 4; i++) {
        for (int j = 0; j < 4; j++) {
            if (piece[i].x == originalPiece[j].x && piece[i].y == originalPiece[j].y) {
                goto ROTATE_PIECE_OUTER_CONTINUE;
            }
        }
        chtype ch = mvinch(piece[i].y, piece[i].x);
        if (ch == '#' || ch == L'│' || ch == L'▁' || ch == L'▔') return;
    ROTATE_PIECE_OUTER_CONTINUE:
        continue;
    }
    for (int i = 0; i < 4; i++) {
        mvaddch(originalPiece[i].y, originalPiece[i].x, ' ');
    }
    for (int i = 0; i < 4; i++) {
        mvaddch(piece[i].y, piece[i].x, '#');
    }
    refresh();
    memcpy(originalPiece, piece, sizeof(struct Coords) * 4);
}

bool update_piece_on_screen(struct Coords *piece, enum Direction dir) {
    switch (dir) {
    case Down:
        for (int i = 0; i < 4; i++) {
            for (int j = 0; j < 4; j++) {
                if (i == j) continue;
                if (piece[i].x == piece[j].x && piece[i].y == piece[j].y - 1) goto UPDATE_PIECE_DOWN_CONTINUE;
            }
            chtype adjCh = mvinch(piece[i].y + 1, piece[i].x);
            if (adjCh == '#' || adjCh == L'▔') return false;
        UPDATE_PIECE_DOWN_CONTINUE:
            continue;
        }
        for (int i = 0; i < 4; i++) {
            mvaddch(piece[i].y, piece[i].x, ' ');
            piece[i].y++;
        }
        for (int i = 0; i < 4; i++) {
            mvaddch(piece[i].y, piece[i].x, '#');
        }
        break;
    case Left:
        for (int i = 0; i < 4; i++) {
            for (int j = 0; j < 4; j++) {
                if (i == j) continue;
                if (piece[i].y == piece[j].y && piece[i].x == piece[j].x + 1) goto UPDATE_PIECE_LEFT_CONTINUE;
            }
            chtype adjCh = mvinch(piece[i].y, piece[i].x - 1);
            if (adjCh == '#' || adjCh == L'│') return false;
        UPDATE_PIECE_LEFT_CONTINUE:
            continue;
        }
        for (int i = 0; i < 4; i++) {
            mvaddch(piece[i].y, piece[i].x, ' ');
            piece[i].x--;
        }
        for (int i = 0; i < 4; i++) {
            mvaddch(piece[i].y, piece[i].x, '#');
        }
        break;
    case Right:
        for (int i = 0; i < 4; i++) {
            for (int j = 0; j < 4; j++) {
                if (i == j) continue;
                if (piece[i].y == piece[j].y && piece[i].x == piece[j].x - 1) goto UPDATE_PIECE_RIGHT_CONTINUE;
            }
            chtype adjCh = mvinch(piece[i].y, piece[i].x + 1);
            if (adjCh == '#' || adjCh == L'│') return false;
        UPDATE_PIECE_RIGHT_CONTINUE:
            continue;
        }
        for (int i = 0; i < 4; i++) {
            mvaddch(piece[i].y, piece[i].x, ' ');
            piece[i].x++;
        }
        for (int i = 0; i < 4; i++) {
            mvaddch(piece[i].y, piece[i].x, '#');
        }
        break;
    default: break;
    }

    return true;
}

int add_new_piece() {
    int piece = 6; // rand() % 7;
    for (int i = 0; i < 4; i++) {
        if (mvinch(pieces[piece][i].y, pieces[piece][i].x) == '#') return -1;
        mvaddch(pieces[piece][i].y, pieces[piece][i].x, '#');
    }
    return piece;
}

void draw_game_board() {
    // mvaddstr is used in place of mvaddch for wide characters because macOS's ncurses is broken
    mvaddch(0, 0, ' ');
    for (int x = 1; x <= BOARD_WIDTH; x++) {
        mvaddstr(0, x, "▁");
    }
    mvaddch(0, BOARD_WIDTH + 1, ' ');

    for (int y = 1; y <= BOARD_HEIGHT; y++) {
        mvaddstr(y, 0, "│");
        for (int x = 1; x <= BOARD_WIDTH; x++) {
            mvaddch(y, x, ' ');
        }
        mvaddstr(y, BOARD_WIDTH + 1, "│");
    }

    mvaddch(BOARD_HEIGHT + 1, 0, ' ');
    for (int x = 1; x <= BOARD_WIDTH; x++) {
        mvaddstr(BOARD_HEIGHT + 1, x, "▔");
    }
    mvaddch(BOARD_HEIGHT + 1, BOARD_WIDTH + 1, ' ');

    refresh();
}

void update_game(enum Direction press) {
    static int downTimer = 0;
    static struct Coords *piece;
    static enum Piece pieceType;
    if (!piece) {
        piece = malloc(sizeof(struct Coords *) * 4);
        srand(time(NULL));
        rand();
        pieceType = add_new_piece();
        memcpy(piece, pieces[pieceType], sizeof(struct Coords) * 4);
    }

    switch (press) {
    case Up: rotate_piece(piece, pieceType); break;
    case Left:
    case Right: update_piece_on_screen(piece, press); break;
    case Down: goto MOVE_DOWN;
    case None: break;
    }

    downTimer++;
    if (downTimer >= 50) {
    MOVE_DOWN:
        downTimer = 0;
        if (!update_piece_on_screen(piece, Down)) {
            tetris_check();
            pieceType = add_new_piece();
            if (pieceType == -1) { // Game over
                ncurses_deinit();
                printf("%s\n", "Game over");
                exit(0);
            }
            memcpy(piece, pieces[pieceType], sizeof(struct Coords) * 4);
            for (int i = 0; i < 4; i++) {
                mvaddch(piece[i].y, piece[i].x, '#');
            }
        }
    }

    refresh();
}

ncurses_util.h:

#ifndef TETRIS_NCURSES_UTIL_H
#define TETRIS_NCURSES_UTIL_H

void ncurses_init();
void ncurses_deinit();

void ncurses_should_exit(int signalValue);

#endif /* TETRIS_NCURSES_UTIL_H */

ncurses_util.c:

#include "ncurses_util.h"

#include <locale.h>
#include <ncurses.h>
#include <signal.h>
#include <stdlib.h>

void ncurses_init() {
    // Default cleanup for ^C, etc.
    signal(SIGINT, ncurses_should_exit);
    signal(SIGTERM, ncurses_should_exit);
    signal(SIGQUIT, ncurses_should_exit);

    setlocale(LC_ALL, "");
    initscr();
    start_color();
    use_default_colors();
    cbreak();
    nodelay(stdscr, TRUE);
    keypad(stdscr, TRUE);
    noecho();
}

void ncurses_deinit() {
    endwin();
}

void ncurses_should_exit(int signalValue) {
    ncurses_deinit();
    exit(128 + signalValue);
}

Some of the aspects I'm most interested in:

  • I use goto in several places to escape nested loops. Is this use acceptable, or is there a cleaner way to do this?
  • Are there any (common) mistakes that I've made?
  • My comments are relatively sparse; is this OK or should I comment more thoroughly?
  • Is my code generally readable/understandable?

For this code to properly function on macOS, you must compile it with -D_XOPEN_SOURCE_EXTENDED -lncurses. On Linux, you must compile it with -lncursesw. This is due to its usage of wide characters.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Code copied into the body of the question is required, please. \$\endgroup\$ – Reinderien Nov 24 '20 at 3:57
  • \$\begingroup\$ @Reinderien Thanks for letting me know; I added the code to the question body. \$\endgroup\$ – atirit Nov 24 '20 at 4:02
  • 1
    \$\begingroup\$ The only thing I can say is that your coding style and indention is too messy and inconsistent for me to even bother reading the code. update_piece_on_screen is for example some serious spaghetti code, add the lack of new line after each if statement and it turns completely unreadable. \$\endgroup\$ – Lundin Nov 25 '20 at 7:36
  • \$\begingroup\$ @Lundin The lack of newlines after non-braced ifs is a formatting choice that I prefer; I have it set this way in my clang-format rules and what I've read online indicates that (the lack of) a newline there is up to preference. Would you mind explaining what about my coding style and indentation are messy/inconsistent? \$\endgroup\$ – atirit Nov 25 '20 at 7:40
  • \$\begingroup\$ It is simply not commonly used style at all, so to someone who is used at reading a whole lot of code written with common code styles, it becomes very difficult to read. Particularly without a new line after statement - if I see if(something) then I expect that the code on the next line after if belongs to the if. Not some completely unrelated code. Also, your switch formatting is weird, but that's a minor remark. \$\endgroup\$ – Lundin Nov 25 '20 at 7:55
1
\$\begingroup\$

Assignment-in-expression

Whereas this -

while ((ch = getch()))

is a "common" paradigm in C, that doesn't mean it's a great idea. You're better-served by splitting it into separate assignment and condition-check statements.

Helpful constants

It's good that you wrote a comment here:

usleep(20000); // 50FPS

it would be even nicer if you had constants that derive this programatically, i.e.

#define FRAME_RATE 50
#define FRAME_TIME_US (1000000 / FRAME_RATE)

or equivalent const declarations.

typedefs

It's generally more usable to add utility typedefs to your enum and struct declarations so that you can (for example) drop the enum in

enum Direction dir;

State management

The way you manage state is interesting. The state of the game is (from what I can tell) entirely represented in your display term buffer. That's not crazy, but it's a measure toward a level of efficiency that in this context is really not necessary, and harms your application in other categories.

What if you wanted to play a "headless" game? What if you wanted to run unit tests in the absence of a real terminal? What if you have a real terminal, but you want to quickly page between different game instances? Your current implementation makes all of these very difficult.

Consider prying apart your application layer from your presentation layer. Rather than representing the game state as display character data, represent it as the best "logical" two-dimensional array of booleans, or maybe bitfields if you want to be fancy. Also, eliminate statics such as

static char *fullLine;
static char *emptyLine;

with prejudice. They will introduce surprising side-effects for anyone attempting to use your code in a re-entrant manner.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you so much! I had a few clarifying questions (I'm not doubting anything you said; I just would like to understand better): 1. You mention that assignment in the while condition isn't the best idea. What's the reason for this? 2. For fullLine, I wasn't sure how to handle representing that once I refactored to allow a variable-sized game board. What would be a better way of doing that? If I could use a #define I would. 3. For the other statics, I figured that it'd be OK to use them to store the game state since they'd be polled/modified on each call anyway. Would globals be better? \$\endgroup\$ – atirit Nov 24 '20 at 18:13
  • 1
    \$\begingroup\$ 1. Clarity and vulnerability to human error. Writing it as you have doesn't make a performance difference to any optimizing compiler; the issue is that you're doing two things at once, and one of the things has a side-effect. \$\endgroup\$ – Reinderien Nov 24 '20 at 18:43
  • 1
    \$\begingroup\$ 2. I don't think you need fullLine in memory at all. For the comparison, use a simple == within a loop; and for output of a full line you can use one of the ncurses window-based functions. \$\endgroup\$ – Reinderien Nov 24 '20 at 18:50
  • 1
    \$\begingroup\$ Would globals be better? Nope. Consider making a struct to represent your game state and passing it around to functions that need it. \$\endgroup\$ – Reinderien Nov 24 '20 at 18:51
  • 1
    \$\begingroup\$ Thanks for elaborating. I really do appreciate it. 🙂 \$\endgroup\$ – atirit Nov 24 '20 at 19:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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