Tic Tac Toe in C

Question

would appreciate if anyone could look through my tic tac toe code and what I possibly do to improve upon it.

Initializing the Board

void BoardInitializer(char board[HORIZONTAL][VERTICAL])
{
    char index = '1';
    for (int i = 0; i < 3; i++)
    {
        for (int j = 0; j < 3; j++)
        {
            board[i][j] = index;
            index++;
        }
    }
}

Printing the board

void DrawBoard(char board[HORIZONTAL][VERTICAL])
{
    for (int i = 0; i < 3; i++)
    {
        for (int j = 0; j < 3; j++)
        {
            printf("%c", board[i][j]);
            if (j != 2)
                printf("|");
        }
        if (i != 2)
            printf("\n---------");
        printf("\n");
    }
}

Determining the position of X & Os

int PositionCalculation(char board[3][3], int selection, char input)
{
    int column = selection % 3;
    int row = selection / 3;

    if (column == 0)
    {
        column += 3;
        row -= 1;
    }

    if (board[row][column - 1] == 'o' || board[row][column - 1] == 'x')
        return -1;

    board[row][column - 1] = input;
    return 0;
}

Win condition checks

bool HorizontalCheck(char board[HORIZONTAL][VERTICAL])
{
    for (int i = 0; i < HORIZONTAL; i++)
    {
        if (board[i][0] == board[i][1] && board[i][1] == board[i][2])
            return true;
    }
}

bool VerticalCheck(char board[HORIZONTAL][VERTICAL])
{
    for (int i = 0; i < VERTICAL; i++)
    {
        if (board[0][i] == board[1][i] && board[1][i] == board[2][i])
            return true;
    }
}

bool DiagonalCheck(char board[HORIZONTAL][VERTICAL])
{
    if ((board[0][0] == board[1][1] && board[1][1] == board[2][2]) || (board[0][2] == board[1][1] && board[1][1] == board[2][0]))
        return true;
}

The main program

#define HORIZONTAL 3
#define VERTICAL 3

void TicTacToe()
{

int selection = 0;
int counter = 0;
int checker = 0;
char board[HORIZONTAL][VERTICAL];


BoardInitializer(board);
DrawBoard(board);

while (counter != 9)
{
    if (counter % 2 == 0)
    {
        printf("\nPlease input where you want to place X: ");
        scanf(" %d", &selection);

    SCANX:
        while (selection < 0 || selection > 10)
        {
            printf("Invalid input, please try again: ");
            scanf(" %d", &selection);
        }

        printf("\n");
        checker = PositionCalculation(board, selection, 'x');

        if (checker == -1)
        {
            selection = -1;
            goto SCANX;
        }

        DrawBoard(board);
        counter++;

        if (counter > 3)
        {
            if (HorizontalCheck(board) == true || VerticalCheck(board) == true || DiagonalCheck(board) == true)
                goto XWINS;
        }
    }
    else
    {
        printf("\nPlease input where you want to place O: ");
        scanf(" %d", &selection);

    SCANO:
        while (selection < 0 || selection > 10)
        {
            printf("Invalid input, please try again: ");
            scanf(" %d", &selection);
        }
        printf("\n");

        checker = PositionCalculation(board, selection, 'o');
        if (checker == -1)
        {
            selection = -1;
            goto SCANO;
        }

        DrawBoard(board);
        counter++;

        if (counter > 3)
        {
            if (HorizontalCheck(board) == true || VerticalCheck(board) == true || DiagonalCheck(board) == true)
                goto OWINS;

        }
    }
}
printf("Its a draw\n");
goto ENDPROGRAM;

XWINS: 
    printf("\nX wins!\n");
    goto ENDPROGRAM;
OWINS: 
    printf("\nO wins\n");
    goto ENDPROGRAM;

ENDPROGRAM: 
    return;
}

Answer 1

Good job in general!

Avoid using goto unless absolutely necessary

Gotos make code really hard to follow, and difficult to refactor. You should really consider changing that whole approach.

A simple "enum and switch"-based state machine would work just as well, and be a lot more straightforward.

Edit Example:

enum GameState {
  X_PLAY,
  O_PLAY,
  X_WON,
  O_WON,
};

GameState state = X_PLAY;

while (counter != 9) {
  switch(state) {
  case X_PLAY:
    ...
    break;
  case O_PLAY:
    ...
    break;
  };
}

Avoid using magic values

Most of your constants should de defined as global variables or defines. What would happen if you wanted to change the x and o to the upper-case letters? That refactor would require multiple changes around the code, and be error-prone.

Always use braces with your if/while/for

I find it just too easy to accidentally make a mistake when refactoring code that does not use braces for single-statement ifs.

That's one of the main reason I personally really like the "opening brace on the if line" coding convention. It makes adding the braces have very little influence on code length.

if (j != 2)
  printf("|");
// vs
if (j != 2) {
  printf("|");
}

The only exception to this would be if -> else if sequences:

if(a) {
  ...
} else if(b) {
  ...
} else {
  ...
}

You are conflating the game representation and the visual representation

You are using your graphic representation as your game state. Generally, that's frowned upoon, as it breaks isolation of concerns, and forces you to think about the game state when manipulating the visual representation and vice-versa.

This is definitely a bit contentious, as it's so very convenient for tic-tac-toe, that there is value in it.

Answer 2

Initializing the Board

void BoardInitializer(char board[HORIZONTAL][VERTICAL])
{
    char index = '1';
    for (int i = 0; i < 3; i++)
    {
        for (int j = 0; j < 3; j++)
        {
            board[i][j] = index;
            index++;
        }
    }
}

A couple of points here:

1. Put a newline right before the name of your function:

   void 
   BoardInitializer
   

   This is an older coding style, but one that serves a useful purpose - it makes it possible to find your function definitions using /^name.

2. BoardInitializer is a description, not a name. Try to use imperative phrases in your function names: InitializeBoard or BoardSetup.

3. You have these nice constants: HORIZONTAL, VERTICAL but you don't use them! What is "3"?

4. i and j are not meaningful names. How about row or r or y and col or c or x? Especially since you're not using the HORIZONTAL and VERTICAL constants, the only way to understand what i or j represent is to search for the one line in the function where they are used, and map that against the signature:

   board[i][j] = index;  // char board[HORIZONTAL][VERTICAL]
   

   So I'd suggest board[col][row] = index;

5. You hard-code the whole 1-9 thing. I'm not sure that there's much value in pretending to have any flexibility on the size of the board.

Printing the Board

void DrawBoard(char board[HORIZONTAL][VERTICAL])
{
    for (int i = 0; i < 3; i++)
    {
        for (int j = 0; j < 3; j++)
        {
            printf("%c", board[i][j]);
            if (j != 2)
                printf("|");
        }
        if (i != 2)
            printf("\n---------");
        printf("\n");
    }
}

1. Are you tired of typing char board[HORIZONTAL][VERTICAL] yet? Maybe a typedef of board_t would be a worthwhile investment.

2. The name is good.

3. You've got the same i,j problem as above.

4. Your HORIZONTAL and VERTICAL seem backwards. According to your code, j controls the printing of the items in different columns, within the same row. To me, that suggests that the limit of j would be the HORIZONTAL size, which means that the array should be board[VERTICAL][HORIZONTAL].

Determining the position of X & Os

int PositionCalculation(char board[3][3], int selection, char input)
{
    int column = selection % 3;
    int row = selection / 3;

    if (column == 0)
    {
        column += 3;
        row -= 1;
    }

    if (board[row][column - 1] == 'o' || board[row][column - 1] == 'x')
        return -1;

    board[row][column - 1] = input;
    return 0;
}

1. This is just wrong. The result of this function is not a calculated position. The result of this function is either 0 or -1, with a side effect of storing the input character in the board.

2. Also, returning 0 on success is backwards. You should be returning 0 on failure, non-zero on success.

3. I notice here that you have abandoned HORIZONTAL and VERTICAL in favor of [3][3]. I still think a typedef is a better idea.

4. Your computations are strange. You are trying to map to a row/column position, but your column is going to be in the range (1..3). But you are subtracting 1 from column when you index the board. Wouldn't it be easier to just compute column in the range (0..2) directly?

5. You don't range-check the selection variable.

6. The names selection and input are not helpful. Once again, I'm left reading the code to understand what's happening. Why not use names like position, location, or tile_number and player, character, or symbol for the two pieces of data?

Win condition checks

bool HorizontalCheck(char board[HORIZONTAL][VERTICAL])
{
    for (int i = 0; i < HORIZONTAL; i++)
    {
        if (board[i][0] == board[i][1] && board[i][1] == board[i][2])
            return true;
    }
}

bool VerticalCheck(char board[HORIZONTAL][VERTICAL])
{
    for (int i = 0; i < VERTICAL; i++)
    {
        if (board[0][i] == board[1][i] && board[1][i] == board[2][i])
            return true;
    }
}

bool DiagonalCheck(char board[HORIZONTAL][VERTICAL])
{
    if ((board[0][0] == board[1][1] && board[1][1] == board[2][2]) || (board[0][2] == board[1][1] && board[1][1] == board[2][0]))
        return true;
}

1. We're back to the [HORIZONTAL][VERTICAL] approach.

2. You don't actually return false in your checks. Your code is "falling off the end" and is vulnerable to whatever happens to be in the return register. (Probably the result of the last condition tested, but still...)

3. You have written functions to do these checks, but you're missing one function. Instead of putting calls to HorizontalCheck(board) || VerticalCheck(board) || DiagonalCheck(board) in your main routine, why not write a function called GameOver(board) and summarize the results?

The main program

I'm not going to copy/paste the entire thing. (whew!)

1. selection doesn't need to be initialized, since you scanf into it.

2. checker exists only to hold the result of your PositonCalculator function for one line. Eliminate it, and rewrite PositionCalculator.

3. counter can be eliminated if you move your victory-check functions under a single GameOver function.

4. The SCANX label is just a while loop. Change your code to make it easier to read.

   while (!apply_move(location, symbol)) {
       printf(...)
       scanf(...)
   }
   

5. You perform a computation to determine if the current player is 'X' or 'O'. But you then divide your code into two sections, basically identical except for using literal 'x' or 'o' characters. Why not do the computation, store the 'x' or 'o' into a variable, and shrink your code by 50%?

6. There are a LOT of goto statements that are really just laziness on your part. (Or a strange desire to use goto statements.) You can eliminate the whole XWINS and OWINS things by simply moving that code up to the one place where it gets used:

               if (HorizontalCheck(board) == true || VerticalCheck(board) == true || DiagonalCheck(board) == true)
                   goto OWINS;
   
           }
       }
   }
   printf("Its a draw\n");
   goto ENDPROGRAM;
   
   XWINS: 
       printf("\nX wins!\n");
       goto ENDPROGRAM;
   OWINS: 
       printf("\nO wins\n");
       goto ENDPROGRAM;
   
   ENDPROGRAM: 
   return;
   

   Becomes:

               if (HorizontalCheck(board) == true || VerticalCheck(board) == true || DiagonalCheck(board) == true) {
                   printf("\nO wins\n");
                   goto ENDPROGRAM;
               }
           }
       }
   }
   printf("Its a draw\n");
   ENDPROGRAM: 
   return;
   

   And since goto ENDPROGRAM really just means return is simplifies to:

               if (HorizontalCheck(board) == true || VerticalCheck(board) == true || DiagonalCheck(board) == true) {
                   printf("\nO wins\n");
                   return;
               }
           }
       }
   }
   printf("Its a draw\n");
   

7. There's no reason to have a void return just before the closing curly brace of a function. (Except to use as a statement for a label, which isn't needed.)