3
\$\begingroup\$

I have implemented a Tic-Tac-Toe game in C which can be played as two-player or with a computer player.

#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>
#include <stdlib.h>
#include <ctype.h>



#define NPLY 10

/* board is represented using two bitfields */
const int wins[] = { 0007, 0070, 0700, 0111, 0222, 0444, 0421, 0124 };

int bestm[NPLY];

void show_board(int x, int y);

char next_player(char d)
{
    return (d == 'x') ? 'o' : 'x';
}

/* determine if there are three in a row */
int is_win(int n)
{
    for (size_t i = 0; i < sizeof(wins) / sizeof(int); i++) {
    if ((n & wins[i]) == wins[i])
        return true;
    }
    return false;
}

/* determine best move using recursive search */
int negamax(int d, int x, int o, int ply)
{
 
    int bestmove;
    int value = -999, cur;
    int xx, oo;
    if (is_win(x))
      return (d == 'x') ? 1 : -1;
    else if (is_win(o))
      return (d == 'o') ? 1 : -1;
    else if ((x | o) == 0777)   /* all squares filled, draw */
    return 0;
    for (int i = 0; i < 9; i++) {
      xx = x; oo = o;
      if ((x | o) & (1 << i))
    continue;
      if (d == 'x') /* make move */
    xx |= (1 << i);
      else
    oo |= (1 << i);
      cur = -negamax(next_player(d), xx, oo, ply - 1);
      if (cur > value) {
    value = cur;
    bestmove = i;
      }
    }
    bestm[ply] = bestmove;
    return value;
}


void show_board(int x, int o)
{
    puts("+---+---+---+");
    for (int i = 0; i < 3; i++) {
    putchar('|');
    for (int j = 0; j < 3; j++) {
        char c;
        if (x & (1 << (3 * i + j)))
        c = 'X';
        else if (o & (1 << (3 * i + j)))
        c = 'O';
        else
        c = '0' + 3 * i + j;
        printf(" %c |", c);
    }
    puts("\n+---+---+---+");
    }
}



int main()
{
    int x = 0, o = 0;
    char c, d = 'x';
    char ai;
    bool over = false;
    int move;
    char s[100];
    printf("Enter AI player x/o or 2 for two player: ");
    ai = tolower(getchar());
    while ((c = getchar()) != '\n');

    while (!over) {


    if (d == ai) {
        negamax(d, x, o, NPLY - 1);
        move = bestm[NPLY - 1];
    } else {
        show_board(x, o);
        printf("Player %c, make your move: ", d);
        fflush(stdout);


        fgets(s, sizeof(s), stdin); /* read a line */
        move = atoi(s);

        if (!(0 <= move && move < 9)) {
        puts("Invalid move.");
        continue;
        }
        if ((x & (1 << move)) || (o & (1 << move))) {
        puts("Square occupied; try again.");
        continue;
        }
    }
    if (d == 'x') {
        x |= (1 << move);
    } else {
        o |= (1 << move);
    }

    d = next_player(d);

    if (is_win(x)) {
        show_board(x, o);
        puts("X wins");
        over = true;
    } else if (is_win(o)) {
        show_board(x, o);
        puts("O wins");
        over = true;
    } else if ((x | o) == 0777) {
        puts("Draw");
        over = true;
    }
    }
}

What points could be improved? What would you have done differently and why?

\$\endgroup\$
1
\$\begingroup\$

Here are some things that may help you improve your code.

Fix your formatting

There are inconsistent spaces at the beginning of lines, inconsistent indentation and inconsistent use and placement of curly braces {}. Being consistent helps others read and understand your code.

Use longer, more meaningful names

Names like c and d are not very descriptive and leave readers of the code little clue as to their significance to the program.

Eliminate global variables

The global variables wins and bestm don't need to be global. Eliminating them allows your code to be more readable and maintainable, both of which are important characteristics of well-written code. Global variables introduce messy linkages that are difficult to spot and error prone. For this program, wins can be a static const variable defined within is_win. The bestm array could also be static to negamax and the move could be an additional parameter like this:

int negamax(int current_player, int x, int o, int ply, int* best) {
    // mostly same
    *best = bestm[ply] = bestmove
    return value;
}

The call from main would then be:

negamax(current_player, x, o, NPLY - 1, &move);

Simplify expressions

The code includes these lines:

if ((x & (1 << move)) || (o & (1 << move))) {
    puts("Square occupied; try again.");
    continue;
}

This could be somewhat simpler as

if ((x|o) & (1 << move)) {

Similarly, show_board could be rewritten as:

void show_board(int x, int o)
{
    int mask = 1;
    int col = 0;
    for (int i = '0'; i < '9'; ++i, mask <<= 1) {
        if (col == 0) {
            printf("\n+---+---+---+\n|");
            col = 3;
        }
        char c;
        if (x & mask) {
            c = 'X';
        } else if (o & mask) {
            c = 'O';
        } else {
            c = i;
        }
        printf(" %c |", c);
        --col;
    }
    puts("\n+---+---+---+");
}

Simplify win checking

The game currently checks if either player won after every move, but this isn't really necessary. By definition, only the player who just moved can win the game. This same strategy can be used within the negamax routine.

Create more helper functions

I'd suggest that the main routine could be made a little more clear if some helper functions were defined and used, such as checking for a tie or checking if a slot is occupied. These would allow the logic of the game to be clear while hiding implementation details.

Consider a different data structure

Right now there are a number of places in the code where we have something like this:

if (d == 'x') {
    x |= (1 << move);
} else {
    o |= (1 << move);
}

If we rename d to current_player and have it point to either x or o, we could write this:

*current_player |= (1 << move);

By creating a structure like this:

struct {
    char token;
    int board;
} players[2] = {
    {'x', 0},
    {'o', 0},
};

We could simplify even further.

Use only required #includes

The code currently has an #include that is not needed. Nothing is used from <stdint.h>. Only include files that are actually needed.

\$\endgroup\$
1
  • \$\begingroup\$ Re: Formatting and includes. Let automatic tool handle that. Not efficient use of programmer's time. \$\endgroup\$ – chux - Reinstate Monica Jun 29 '20 at 2:18

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.