Text Based Minesweeper in C

Question

This is my first C program. The code works, however I'm not yet familiar with how to write idiomatic C. How should I improve this?

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

#define UNCLICKED 0
#define CLICKED 1
#define FLAGGED 2

#define NOT_BOMB 0
#define BOMB 1

struct piece {
    char user_state, game_state, value;
};

int generate_rand();
int valid(int, int);
void print_board(struct piece[8][8], int);
int click(struct piece[8][8], int, int);
int game_over(struct piece[8][8]);

int main() {
    struct piece board[8][8];

    int i, j;
    for (i = 0; i < 8; i++) {
        for (j = 0; j < 8; j++) {
            board[i][j].user_state = UNCLICKED;
            board[i][j].game_state = NOT_BOMB;
            board[i][j].value = 0;
        }
    }

    srand(time(0));

    int num_bombs = 10;

    while (num_bombs > 0) {

        int y, x;

        do {
            y = generate_rand();
            x = generate_rand();
        } while (board[y][x].game_state == BOMB);

        board[y][x].game_state = BOMB;

        int delta_row, delta_column;
        for (delta_row = -1; delta_row <= 1; delta_row++) {
            for (delta_column = -1; delta_column <= 1; delta_column++) {
                if (valid(y + delta_row, x + delta_column)) {
                    board[y + delta_row][x + delta_column].value++;
                }
            }
        }

        num_bombs--;
    }

    print_board(board, 1);

    while (1) {
        char row, column, move_type;
        printf("\nEnter a row: ");
        row = getchar() - 48;
        getchar();
        printf("Enter a column: ");
        column = getchar() - 48;
        getchar();

        if (!valid(row, column)) {
            printf("Location is not on board. Try again: \n");
            continue;
        }

        if (board[row][column].user_state == CLICKED) {
            printf("That square is already open. Try again: \n");
            continue;
        }

        printf("[C]lick or [F]lag? ");
        move_type = getchar();
        getchar();
        printf("\n");

        if (move_type == 'C') {
            if (click(board, row, column)) {
                printf("That was a bomb! \n");
                print_board(board, 0);
                break;
            }
        } else if (move_type == 'F') {
            if (board[row][column].user_state == CLICKED) {
                printf("That square is already open. Try again: \n");
                continue;
            } else {
                board[row][column].user_state = FLAGGED;
            }
        } else {
            printf("Enter either 'C' or 'F'. Try again: \n");
            continue;
        }

        if (game_over(board)) {
            printf("You win! \n");
            print_board(board, 0);
            break;
        } else {
            print_board(board, 1);
        }
    }

    return 0;
}

int generate_rand() {
    return (rand() % 8);
}

int valid(int row, int column) {
    return row >= 0 && row < 8 && column >= 0 && column < 8;
}

void print_board(struct piece board[8][8], int in_game) {
    int i, j;
    for (i = 0; i < 8; i++) {
        for (j = 0; j < 8; j++) {
            if (in_game) {
                if (board[i][j].user_state == CLICKED) {
                    printf("%d ", board[i][j].value);
                } else if (board[i][j].user_state == FLAGGED) {
                    printf("F ");
                } else {
                    printf("- ");
                }
            } else {
                if (board[i][j].game_state == NOT_BOMB) {
                    if (board[i][j].user_state == FLAGGED) {
                        printf("X ");
                    } else {
                        printf("%d ", board[i][j].value);
                    }
                } else {
                    printf("B ");
                }
            }
        }
        printf("\n");
    }
}

int click(struct piece board[8][8], int row, int column) {
    if (!valid(row, column) || board[row][column].game_state == BOMB || board[row][column].user_state == CLICKED) {
        return 1;
    }

    board[row][column].user_state = CLICKED;
    if (board[row][column].value == 0) {
        int i, j;
        for (i = -1; i <= 1; i++) {
            for (j = -1; j <= 1; j++) {
                click(board, row + i, column + j);
            }
        }
    }

    return 0;
}

int game_over(struct piece board[8][8]) {
    int i, j;
    for (i = 0; i < 8; i++) {
        for (j = 0; j < 8; j++) {
            if (board[i][j].game_state == NOT_BOMB && board[i][j].user_state == UNCLICKED) {
                return 0;
            }
        }
    }

    return 1;
}

Would it make sense to put some of the code in a separate header file, or is that over complicating things?

Answer 1

One thing that strikes me immediately is that we pass struct piece board[8][8] around quite a lot. There's two things I would change there. The first is to use a typedef (or perhaps a small struct) to save all that typing. The other would be to give names to the dimensions.

#define BOARD_WIDTH  8
#define BOARD_HEIGHT 8

typedef struct piece game_board[BOARD_HEIGHT][BOARD_WIDTH];

If we want to build the game for a different height and width, then there will be only one place to change (once the loop bounds are adjusted to use the defined macros instead of literal 8). And we've made it easier if we ever want to transition to runtime-chosen board size in future (that's a more advanced exercise that you should try when you feel ready for it).

The typedef means that instead of writing out function arguments like

void print_board(const struct piece board[BOARD_HEIGHT][BOARD_WIDTH], int in_game);

they can be shorter and clearer:

void print_board(const game_board board, int in_game);

---

There's some weaknesses here:

    printf("\nEnter a row: ");
    row = getchar() - 48;
    getchar();
    printf("Enter a column: ");
    column = getchar() - 48;
    getchar();

Remember that any call to getchar() can return EOF. If that happens, we'll probably want to terminate the program, as we can't expect any useful input from that point onwards.

Secondly, the magic number 48 is specific to the character coding in use - we can make the program more portable by using '0' as is evidently intended.

It's also quite fragile - we're expecting a single character and newline, but users make mistakes and might enter multiple characters (and we'd probably want to allow that if we make the board bigger than 10✕10).

Thankfully, we have a better alternative to char-by-char input, and that's formatted input using scanf(). That would look something like

    printf("\nEnter a row and column: ");
    while (scanf("%hhd%hhd", &row, &column) != 2) {
        if (feof(stdin)) {
            /* scanf can never succeed now */
            fputs("Input failure\n", stderr);
            return EXIT_FAILURE;
        }
        /* Discard any remaining input */
        scanf("%*[^\n]");   /* ignore errors here */
        printf("Invalid input.  Enter a row and column: ");
    }

That's not 100% robust, but it's an improvement.

---

A small fix: these function declarations should be prototypes, i.e. specify what arguments they take:

int generate_rand();

int main() {

The way we tell the compiler that they take no arguments at all (rather than unspecified arguments) is to write void between the parentheses:

int generate_rand(void);

int main(void) {

---

The structure is a bit unbalanced. There are a few low-level functions, but most of the logic is in a huge main(). Try dividing it into high-level functions - perhaps initialise_board() could be one of them?

---

There are more things that could be improved, but I hope this gives you some things to consider.

Answer 2

Some style suggestions:

1. Use enums instead of #defines.

   enum {
     state_UNCLICKED,
     state_CLICKED,
     state_FLAGGED
   } ;
   

Followed by if (board[i][j].user_state == state_CLICKED)

2. It's easier to understand structure definitions if each member is on its own line.

struct piece {
   char user_state;
   char game_state;
   char  value;
};

3. Generally it's nice to put argument names in function declarations. Then readers don't need to jump to the implementation to find out what the arguments mean.

4. I might refactor

           board[i][j].user_state = UNCLICKED;
           board[i][j].game_state = NOT_BOMB;
           board[i][j].value = 0;

into a board_square_init(board, i,j) function. I might also create a square_place_bomb(board, i,j) function that does the neighbor update.

5. It's odd that click returns 1 for several unrelated things: out of bounds, already clicked, and bomb. You are getting away with it because you pre-test for clicked before calling this function, but it would be cleaner if you returned a different value for the different failures. Maybe add state_INVALID to the enum in comment 1, and return a member of the enum.

6. Many people dislike globals in C, and there are some good reasons for that. But unless you are planning on writing a program that manages multiple boards at once, you could just declare the single board once globally, and stop passing it to every single function. You trade a potentially useful future capability of multiple board support for simpler code today.