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.

• wed1may: This is great feedback, lots of useful tips here which I didn't know about at all. It's great to see how many improvements can be made in even a very small program. Thanks for taking the time to review it :)
• wed1may: I didn't know we can use a ternary like that in printf, that's cool.
• Madagascar: @wed1may Welcome to C, I look forward to seeing more of your questions.