Tic Tac Toe Basic C implementation

Question

• Is my implementation idiomatic?
• Does it suffer from any undefined behaviour?

Any pointers/tips would be greatly appreciated. Particularly, I think that the winner function could be improved.

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

/* ascii values */
#define X 88
#define O 79

void print_board(char* b) {
    char buf[19] = {0};
    int p = 0;
    for (int i = 0; i < 9; i++) {
        buf[p++] = b[i] == 0 ? i + '1' : b[i];
        buf[p++] = (i + 1) % 3 == 0 ? '\n' : ' ';
    }
    printf("%s", buf);
}

/* 0 is ongoing; 1 is draw. */
char winner(char* b) {
    char pieces[2] = {X, O};
    for (int i = 0; i < 2; i++) {
        char p = pieces[i];
        if (/* rows */
            b[0] == p && b[1] == p && b[2] == p ||
            b[3] == p && b[4] == p && b[5] == p ||
            b[6] == p && b[7] == p && b[8] == p ||
            /* cols */
            b[0] == p && b[3] == p && b[6] == p ||
            b[1] == p && b[4] == p && b[7] == p ||
            b[2] == p && b[5] == p && b[8] == p ||
            /* diagonals */
            b[0] == p && b[4] == p && b[8] == p ||
            b[2] == p && b[4] == p && b[6] == p)
            return p;
    }
    for (int i=0; i < 9; i++)
        if (b[i] == 0)
            return 0;
    return 1;
}

char getmove(char player, char* b) {
    char c;
    int ok = 0;
    do {
        printf("%c to move: ", player);
        for (c = getchar(); getchar() != '\n';)
            ;
        putchar('\n');
        if (c < '0' || c > '9')
            printf("Expected a number 0-9\n");
        else if (b[c - '0' - 1])
            printf("Square %c is taken\n", c);
        else
            ok = 1;
    } while (!ok);
    return c - '1';
}

void makemove(char player, int pos, char* b) {
    assert(pos >= 0 && pos < 9);
    b[pos] = player;
}

void print_winner(char result) {
    assert(result);
    if (result == 1)
        printf("It's a draw!\n");
    else
        printf("%c wins!\n", result);
}

int main(void) {
    char player = X;
    char move;
    char board[9] = {0};
    int status;
    while (!(status = winner(board))) {
        print_board(board);
        move = getmove(player, board);
        makemove(player, move, board);
        player = player == X ? O : X;
    }
    print_board(board);
    print_winner(status);
    return 0;
}

Answer 1

getchar() returns an int, not a char:

Defined in header <stdio.h>

int getchar( void );

Reads the next character from stdin.

Equivalent to getc(stdin).

See: Difference between int and char in getchar/fgetc and putchar/fputc?.

Eliminate magic values from the code:

/* ascii values */
#define X 88
#define O 79

But what do 88 and 79 signify? Why not define an enum instead?

char buf[19] = {0};

Why 19? This would be better as:

#define BOARD_SIZE 19

// Or, C24 and above
constexpr unsigned int board_size = 19; // Or deduce the type with auto

Similarly, I see the naked value 9 being repeated multiple times. It should be some named constant as well.

Similarly, define some named constants for "draw" and "ongoing" instead of using 0 and 1.

Only declare variables where required:

move = getmove(player, board);
makemove(player, move, board);

We don't need move here. Simply do:

makemove(player, getmove(player, board), board);

Prefer using standard exit status codes instead of magic values:

stdlib.h defines EXIT_SUCCESS and EXIT_FAILURE. return 0 is better as return EXIT_SUCCESS.

Simplify:

void print_winner(char result) {
    assert(result);
    if (result == 1)
        printf("It's a draw!\n");
    else
        printf("%c wins!\n", result);
}

can be written as:

void print_winner(char result) {
    assert(result);
    printf(result == 1 ? "It's a draw!\n" : "%c wins!\n", result);
}

In winner() (this could be named better):

for (int i=0; i < 9; i++)
        if (b[i] == 0)
            return 0;
return 1;

can be replaced with:

#include <string.h>

return memchr(b, 0, 9) != NULL;

Error messages go to stderr:

if (c < '0' || c > '9')
    printf("Expected a number 0-9\n");
else if (b[c - '0' - 1])
    printf("Square %c is taken\n", c);

Assuming these are actually errors, they should be going to stderr (with fputs()/fprintf()) instead of stdout.

Use const for data that is not modified:

In print_board(), winner(), and getmove(), b is not modified anywhere in the code. So it should be declared with the const qualifier.

b should also be named appropriately. It would be better as board.

Also note that char* b is C++ style. C style is char *b.

Use puts() where appropriate:

printf("%s", buf);

We do not require printf()'s formatting capabilities here, so puts() would be more appropriate to use here, even if most compilers would make that switch for you automatically.