12
\$\begingroup\$

I'm currently learning C, and decided to practice what I've been working on with a project, namely implementing Conway's Game of Life as a terminal visualization. This is my first project in C.

The result: result of the code

and my code:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/ioctl.h>

#define DEAD ' '
#define ALIVE "█" 

int WIDTH, HEIGHT;

typedef enum {
    alive,
    dead,
} cell_t;

cell_t** grid;
cell_t** new_grid;

void clear_screen() {
    printf("\x1b[2J");
}

void hide_cursor() {
    printf("\x1b[?25l");
}

void show_cursor() {
    printf("\x1b[?25h");
}

void move_home() {
    printf("\x1b[0;0H");
}

void move_to(int row, int col) {
    printf("\x1b[%d;%dH", row + 1, col * 2 + 1);
}

void free_grids() {
    for (int i = 0; i < HEIGHT; ++i) {
        free(grid[i]);
        free(new_grid[i]);
    }
    free(grid);
    free(new_grid);
}

int count_neighbors(int row, int col) {
    int count = 0;

    for (int y = row - 1; y <= row + 1; ++y) {
        for (int x = col - 1; x <= col + 1; ++x) {
            if (y >= 0 && y < HEIGHT && x >= 0 && x < WIDTH && !(y == row && x == col)) {
                count += (grid[y][x] == alive) ? 1 : 0;
            }
        }
    }

    return count;       
}

void init_grid() {
    grid = (cell_t**)malloc(HEIGHT * sizeof(cell_t*));
    new_grid = (cell_t**)malloc(HEIGHT * sizeof(cell_t*));

    for (int i = 0; i < HEIGHT; ++i) {
        grid[i] = (cell_t*)malloc(WIDTH * sizeof(cell_t));
        new_grid[i] = (cell_t*)malloc(WIDTH * sizeof(cell_t));

        for (int j = 0; j < WIDTH; ++j) {
            grid[i][j] = (rand() % 2) ? alive : dead; 
        }
    }
}

void print_grid() {
    for (int y = 0; y < HEIGHT; ++y) {
        for (int x = 0; x < WIDTH; ++x) {
            if (grid[y][x] == alive) {
                move_to(y, x);
                int n = count_neighbors(y, x);
                if (n < 2) {
                    // underpopulation
                    printf("\x1b[36m");
                } else if (n == 2 || n == 3) {
                    // good
                    printf("\x1b[34m");
                } else if (n > 3) {
                    // overpopulation 
                    printf("\x1b[35m");
                }

                printf(ALIVE);
                printf(ALIVE);
                printf("\x1b[0m");
            }
        }

        if (y < HEIGHT - 1) {
            printf("\n");
        }
    }

    fflush(stdout);
}

void next() {
    for (int y = 0; y < HEIGHT; ++y) {
        for (int x = 0; x < WIDTH; ++x) {
            int neighbors = count_neighbors(y, x);

            if (grid[y][x] == alive) {
                new_grid[y][x] = (neighbors == 2 || neighbors == 3) ? alive : dead;
            } else {
                new_grid[y][x] = (neighbors == 3) ? alive : dead;
            }
        }
    }

    for (int y = 0; y < HEIGHT; ++y) {
        for (int x = 0; x < WIDTH; ++x) {
            grid[y][x] = new_grid[y][x];
        }
    }
}

void init_terminal_size() {
    struct winsize w;
    ioctl(STDOUT_FILENO, TIOCGWINSZ, &w);
    WIDTH = w.ws_col / 2;
    HEIGHT = w.ws_row;
}

int main() {
    init_terminal_size();
    init_grid();
    hide_cursor();

    atexit(free_grids);
    atexit(show_cursor);

    while (1) {
        move_home();
        clear_screen();
        print_grid();
        usleep(100 * 1000); 
        next();
    }

    return 0;
}

I'd like some feedback/suggestions on how I can improve it.

The main issues I can identify are:

  1. Blinking (probably due to the amount of printf statements I am making)
  2. Lack of proper program termination
  3. Possibly leaking memory as a result of problem 2
  4. Usage of preprocessor directives (DEAD isn't used-- forgot to delete it, but I'm not sure if I should be using string constants instead).

Trying to fix issue 1, I attempted to create a char* that I could append to using reallocations and just print out the entire char* after I'd appended every character to the dynamic char*. This didn't work and I had to revert it. Additionally, with this method I'd be worried about the amount of allocations I'm making (not sure if this would impact performance or increase performance).

I currently try to use atexit() to free the memory at exit, but the cursor isn't being shown on exit so I can assume that neither of those functions are being executed.

Finally, I attempted to use getchar() to check if q was being pressed on the keyboard, but this halted the program and wouldn't exit. I just realized this was probably because I hadn't entered raw mode.

Advise is appreciated.

\$\endgroup\$
4
  • 4
    \$\begingroup\$ Cool first project. \$\endgroup\$ Dec 31, 2023 at 21:28
  • \$\begingroup\$ @MartinYork Thanks! Obviously I've coded before, starting with Javascript, then branching out to Rust. Although this project has pretty apparent shortcomings, I'm still quite proud of the result. \$\endgroup\$ Jan 1 at 0:43
  • 1
    \$\begingroup\$ Your ALIVE macro is a string, while DEAD is a character. That looks fishy. \$\endgroup\$
    – Jens
    Jan 1 at 13:35
  • \$\begingroup\$ @Jens That's true. Previously, I was printing out the literal " " (2 spaces) so that I didn't have to call printf twice. However, in this code, I didn't even use DEAD because I just move_to the position of each alive cell so that I only have to call printf on the alive cells. \$\endgroup\$ Jan 1 at 14:27

2 Answers 2

16
\$\begingroup\$

I'm impressed that this is your first project in C -- it's good! What follows are some comments on what's good that you should continue to do and then some suggestions for improvements.

What's good?

There's a lot here that is good, including:

  • good naming of functions and variables
  • decomposing the project into logical functions
  • reasonable program flow
  • thought about freeing memory and restoring the terminal

These are all good things that you should definitely strive to continue.

Possible improvements

These are some suggestions for how you might improve this program.

Gracefully exit the program

You're right that the atexit functions are not called because there's no way to gracefully exit the program. One way to do that would be, as you suspected, to alter the terminal settings. Here's one way to do that by modifying your hide_cursor function:

#include <termios.h>
#include <fcntl.h>

static struct termios oldterm, newterm;
static int oldf;

void hide_cursor(void) {
    printf("\x1b[?25l");
    tcgetattr(STDIN_FILENO, &oldterm); /* grab old terminal i/o settings */
    newterm = oldterm; /* make new settings same as old settings */
    newterm.c_lflag &= ~ICANON; /* disable buffered i/o */
    newterm.c_lflag &= ~ECHO; /* disable echo mode */
    tcsetattr(STDIN_FILENO, TCSANOW, &newterm); /* use these new terminal i/o settings now */
    oldf = fcntl(STDIN_FILENO, F_GETFL, 0);
    fcntl(STDIN_FILENO, F_SETFL, oldf | O_NONBLOCK);
}

This matched with a corresponding alteration in the show_cursor routine:

void show_cursor(void) {
    tcsetattr(STDIN_FILENO, TCSANOW, &oldterm);
    fcntl(STDIN_FILENO, F_SETFL, oldf);
    printf("\x1b[?25h\n");
}

What this does is to allow you to gracefully exit by changing the while (1) to this:

for (int ch = getchar(); ch == EOF; ch = getchar()) {

Tell the compiler when no arguments are expected

For most of these functions that don't take arguments, instead of writing void next() write void next(void). Otherwise the compiler will accept any number of arguments to that function, and that's probably not what you want.

Avoid using global variables

In this case, the use of global variables is not terrible since it's a single file program. However, as you write larger programs, try to avoid global variables. They can hide linkages between different parts of the code and make them harder to maintain and understand. If you must have global variables, make them static to give them file scope rather than global scope.

Don't use ALLCAPS for variables

In C, the traditional use for ALLCAPS is for macros, so unless it's a macro (basically any #define), use conventional naming instead.

Understand that return 0 is optional for main

Unlike every other function that returns an int, main is special and does not require you to specifically write return 0;. If you don't that's the implicit behavior anyway. Some people write it anyway; I prefer to omit it. Either way, you may encounter code that either has it or doesn't, so it's good to know about.

Check for errors

The calls to malloc can fail, and when they do, they return NULL. You should check for that before using the memory.

Consolidate constants

The colors shown for the different numbers of neighbors would be easier to understand and modify if they were pulled out as named constants.

Avoid copies if practical

In this case, the last part of next() copies everything from one array to the other. Rather than doing that, an alternative would be to simply toggle which array is the next one vs. the current one.

Add a bit of variation

By design, rand() starts with a seed value of 0 unless otherwise told, which means that you will get the same pattern every time the program is run. To make it different, but controllable by the user, you could seed with a user-supplied number by modifying the first part of main like this:

int main(int argc, char *argv[]) {
    if (argc > 1) {
        srand(atoi(argv[1]));
    }

Use more modern C

If you are using a compiler that supports at least C11, there are a lot of improvements that you can make without sacrificing readability. Step by step, here are the changes.

Use structures to group data

The height and width and the two cell arrays are central to this program, so we can introduce structures to capture this.

struct dims {
    int width;
    int height;
};

struct Grid {
    int current;
    struct dims dim;
    void* g;
};

You may wonder why g is defined as a void * when what it really contains is cell_t data. We sacrifice a little readability here in exchange for a very useful technique later.

Return a structure

The point to init_terminal_size() is to get the width and height of the terminal. Rather than setting global variables, we can return a structure directly:

struct dims init_terminal_size() {
    struct winsize w;
    ioctl(STDOUT_FILENO, TIOCGWINSZ, &w);
    return (struct dims){ .width = w.ws_col / 2, .height = w.ws_row};
}

Allocate memory as few times as possible

Memory allocation is more efficient and easier to track when we minimize the number of allocations. In this program, we can reduce the number of allocations to a single one. First, in main, we can create and initialize the grids like this:

struct Grid cells = { .dim =init_terminal_size() };
if  (!init_grid(&cells)) {
    return 1;
}

Here's what is inside init_grid:

bool init_grid(struct Grid* grid) { 
    cell_t (*pGrid)[2][grid->dim.height][grid->dim.width] = malloc(sizeof(*pGrid));
    if (!pGrid) {
        return false;
    }
    grid->g = pGrid;
    for (int i = 0; i < grid->dim.height; ++i) {
        for (int j = 0; j < grid->dim.width; ++j) {
            (*pGrid)[0][i][j] = (rand() % 2) ? alive : dead; 
        }
    }
    return true;
}

First, unless you are using a C23 compiler, which has it as a keyword, you'll need to #include <stdbool.h> to get bool.

This line uses syntax that is unfamiliar even to many C programmers, so I'll explain it in some detail:

cell_t (*pGrid)[2][grid->dim.height][grid->dim.width] = malloc(sizeof(*pGrid));

This declares a variable pGrid as a three-dimensional array of cell_t. This is using the much-maligned variable-length array (VLA) syntax, but is not a VLA. (See this ACCU talk for a good explanation of what is wrong with VLA, and also how to use syntax like this.) The two big advantages, are that it greatly simplifies calculating the size because we can use the sizeof() operator to do it for us, and that we can use the natural subscripting syntax to access members. Note that the height and width parameters are only available at runtime; we do not know their values during compiling and so we can't use constants. That's why void* is the type used in the structure declaration.

The first dimension uses the idea above and indicates which of the two grid arrays we're using, so it should be pretty clear that this line is randomly assigning alive or dead to each of the cells in the first array:

(*pGrid)[0][i][j] = (rand() % 2) ? alive : dead; 

We could alternatively use a pointer and simply increment it to the end of the array, but this way shows the intent a bit more clearly and we're not that concerned about performance here, since it's only done once.

Pass pointers to avoid global variables

Using this same syntax as described above, we can easily modify some of the other functions. For example, here is count_neighbors:

int count_neighbors(const struct Grid grid[static restrict 1], int row, int col) {
    const int height = grid->dim.height;
    const int width = grid->dim.width;
    const cell_t (*pGrid)[2][height][width] = grid->g;
    int count = 0;
    for (int y = row - 1; y <= row + 1; ++y) {
        for (int x = col - 1; x <= col + 1; ++x) {
            if (y >= 0 && y < height && x >= 0 && x < width && !(y == row && x == col)) {
                count += ((*pGrid)[grid->current][y][x] == alive) ? 1 : 0;
            }
        }
    }

    return count;       
}

Here the [static restrict 1] means that the pointer, grid in this instance, must have at least one valid instance (static) and further that it must have exactly that many (restrict). This can help the compiler generate optimal code and clearly signals the intent to a programmer reading the code.

The next() function uses this, too, and also changes the current grid to avoid copies as mentioned above, also with a small change to use a switch for clarity:

void next(struct Grid grid[static restrict 1]) {
    const int height = grid->dim.height;
    const int width = grid->dim.width;
    cell_t (*pGrid)[2][height][width] = grid->g;
    for (int y = 0; y < height; ++y) {
        for (int x = 0; x < width; ++x) {
            switch (count_neighbors(grid, y, x)) {
                case 3:  // stay alive or become born
                    (*pGrid)[1 - grid->current][y][x] = alive;
                    break;
                case 2:  // stay unchanged
                    (*pGrid)[1 - grid->current][y][x] = (*pGrid)[grid->current][y][x];
                    break;
                default:  // all other cases 
                    (*pGrid)[1 - grid->current][y][x] = dead;
            }
        }
    }
    grid->current = 1 - grid->current;
}
\$\endgroup\$
8
  • \$\begingroup\$ Also, on my computer, it didn't blink, so I can't comment there. \$\endgroup\$
    – Edward
    Dec 30, 2023 at 23:46
  • \$\begingroup\$ Thank you for the in depth answer! I'll accept this in a day, as I'm curious to see if anyone else has any idea about the blinking. Thanks again :) \$\endgroup\$ Dec 30, 2023 at 23:53
  • 4
    \$\begingroup\$ It's usually good to give it a few days before selecting an answer. You'll often get more and better reviews that way. \$\endgroup\$
    – Edward
    Dec 31, 2023 at 0:25
  • \$\begingroup\$ I think you forgot to add the code that seeds the random number generator. \$\endgroup\$
    – texdr.aft
    Dec 31, 2023 at 23:55
  • \$\begingroup\$ Oh! I hadn't thought about simply switching arrays. I really appreciate the extra edit :) \$\endgroup\$ Jan 1 at 0:48
10
\$\begingroup\$

I can't (yet?) comment on the (excellent) first answer, or I would. But I have a few comments to add to what has already been said:

In Unix you don't need to check the result of malloc, if having the program crash in the case of malloc failure is acceptable behavior. It often is, for user applications like this, but never is for long-term server applications! Handling a malloc failure any more than this means you actually have to HANDLE it, which means come up with a viable Plan B and all that entails. What can/should it continue to do if any particular malloc fails? For many programs the most you might need to do is use xmalloc so the sudden death is at least explained:

void *
xmalloc(size_t size)
{
   void *p;

   if (!(p = malloc(size))) {
      write(2, "Out of memory\n", 14);
      abort();
   }
   return p;
}

Graceful termination can be as easy as having it die with ^C. There are no malloc memory leak problems in any kind of Unix program if it dies, the OS reclaims all in-process resources. You don't need any kind of memory cleanup code if you rely on ^C. That's one reason it's so popular! KISS...

Terminal modes, however, do survive program death. If you've messed with the terminal you do want to put it back to normal at death. If you're using ^C to die, for simplicity, you'll want a signal-catcher. It need do nothing but restore the terminal and then die. This is fairly simple code, usually only a few lines.

However, few functions are signal-safe, which limits what you can do in the ^C signal handler. Global variables are safe for use there, as are things like write(2), exemplified above. (But not usually printf(3), beware.)

You don't need atexit handling, unless your program exits in multiple places, places that you don't necessarily control. A small/simple program like this doesn't need that kind of additional complexity. C programs existed happily and well for decades without atexit.

By all means extract any kind of manifold constant out of the code and into #DEFINEs. Any later maintainer, especially including yourself, will be grateful. No such literal constant should EVER appear in more than one place.

'Blinking' sounds like an artifact of your particular virtual terminal environment. If you were to actually run your program against a real serial terminal it wouldn't blink, per se, but would probably be unusably ugly as screen update would be very much slower than your program's next-generation calculation. Again, artifacts of the actual environment you'd be running in. Back in the day, a full screen update that took only 1 second (from first character printed to last) was considered blazingly fast. Your program is probably assuming/counting on the full screen update being instantaneous. What if it isn't, quite? Is that the 'blink'?

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for your in-depth answer, and I appreciate the insight on the blinking. What you mentioned about the slow writing speed to the console is what I suspected, because when I ran this program on the Terminal program in MacOS instead of what I was previously using, iTerm2, I didn't experience any blinking. \$\endgroup\$ Jan 1 at 0:54
  • \$\begingroup\$ @joeymalvinni: stdout is line-buffered by default, and stdio printf is pretty slow vs. fputs(stdout, string) or fwrite. For higher-speed text output, probably you'd want to prepare one big buffer for a single write system call. I'd guess that's how aalib and libcaca do it, and they're fast enough for 30 FPS video playback in a Konsole or gnome-terminal window on Linux, even on computers from 2 decades ago. Also for ASCII-art animations in demos. \$\endgroup\$ Jan 2 at 22:33
  • \$\begingroup\$ @PeterCordes Thanks so much for the added insight. This makes sense, as I wanted to print the entire screen, but messed up the dynamic memory allocation and didn't really have enough experience with C to debug. I'll have to retry some time. Thanks again for the help :) \$\endgroup\$ Jan 4 at 1:07
  • \$\begingroup\$ Correction: fputs(string, stdout). Unlike variadic functions, the f versions of puts and putchar take the stream last instead of first. \$\endgroup\$ Jan 4 at 1: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.