4×4 Tic-Tac-Toe in C

Question

While trying to teach myself C, I decided to take a stab at writing this Tic-Tac-Toe program on a 4x4 board. It will be great if somebody could review it and let me know of some ways of refining this.

#include <stdio.h>
#include <stddef.h>

typedef struct {
    int xpos;
    int ypos;
} BRDPOS;

void printBoard(int brd_size,char board[][brd_size]){
    int i,j;
    for(i = 0;i < brd_size;i++){
        for(j = 0;j < brd_size;j++){
            if(j == 0) printf("| ");
            printf("%c |",(char)board[i][j]);
            if(j == (brd_size - 1)){
                printf("\n");
            }
        }
    }   
}

BRDPOS getPosition(int brd_size,char board[][brd_size]){
    //Enter Positions X and Y
    BRDPOS pos;
    int x,y;
    do{
        printf("Enter the x-position:");
        scanf("%d",&x);
        printf("\nEnter the y-position:");
        scanf("%d",&y);
        printf("\n");               
    }while(board[x][y] != ' ');
    pos.xpos = x;
    pos.ypos = y;
    return pos;
}

void updateBoard(int brd_size,char board[][brd_size],BRDPOS pos,int player){
    if(player == 0){
        board[pos.xpos][pos.ypos] = 'X';
    }else{
        board[pos.xpos][pos.ypos] = 'O';
    }
}

int isWinningPosition(BRDPOS position,int brd_size,char brd[][brd_size],int player){
    char posChar = (player == 0)? 'X':'Y';
    int xPos = position.xpos;
    int yPos = position.ypos;
    char yStatus = brd[0][0];
    char majDiag = brd[0][0];
    char minDiag = brd[0][brd_size-1];
    int xstatus = 0;
    int ystatus = 0;
    int majdiag = 0; 
    int mindiag = 0;
    int i,j,k,l;
    for(i=0;i< brd_size;i++){
        if(brd[xPos][yPos] == brd[xPos][i]){
            xstatus = 1;
        }else{
            xstatus = 0;
            break;
        }
    }
    //Check if the horizontal has the same characters
    for(j=0;j<brd_size;j++){
        if(brd[xPos][yPos] == brd[j][yPos]){
            ystatus = 1;
        }else{
            ystatus = 0;
            break;
        }
    }
    //Check if the major diagonal has the same characters
    if(xPos == yPos){
        for(k=0;k < brd_size;k++){
            if(majDiag == brd[k][k]){
                majdiag = 1;
            }else{
                majdiag = 0;
                break;
            }
        }
    }
    //Check if the minor diagonal has the same characters
    if(xPos == (brd_size - 1 - yPos)){
            for(l = 1;l < brd_size;l++){
                if(minDiag == brd[l][(brd_size-1) - l]){
                    mindiag = 1;
                } else {
                    mindiag = 0;
                    break;
                }
        }
    }
    return xstatus|ystatus|majdiag|mindiag; 
}

int main()
{
    //Flag to indicate the current player 0 for A, 1 for B. Default to A
    int player = 0;
    //Flag to indicate the winner, -1 for no winner, 0 for A, 1 for B
    int winner = -1;
    //Counter to indicate the total number of turns
    int counter = 0;
    //Tic-Tac-Toe board
    char board[4][4];
    int i,j;
    for(i = 0;i<4;i++){
        for(j = 0;j<4;j++){
            board[i][j] = ' ';
        }
    }
    //Keep iterating until a winner is found and the cell count is less than 16 
    while((winner == -1) && (counter < 16)){
        //Print the tic-tac-toe board
        printBoard(4,board);
        //Ask position to place the X for player 1
        //getPosition keeps asking for the position until a legal position has been entered
        printf("Enter positions for PlayerA:\n");
        BRDPOS positionA = getPosition(4,board);
        //Update board with positionA
        printf("Updating the board:\n");
        updateBoard(4,board,positionA,0);
        //Check if the updated position is a winning position
        if(isWinningPosition(positionA,4,board,0)){
            //Mark player A as the winner
            printf("Marking player A as the winner \n");
            winner = 0;
            break;
        }
        //update the move counter
        counter++;
        //print updated board
        printBoard(4,board);
        //Ask position to place the 'O' for player 2
        printf("Enter position for PlayerB:\n");
        BRDPOS positionB = getPosition(4,board);
        //Update board with positionB
        updateBoard(4,board,positionB,1);
        //Check if the updated position is a winning position       
        if(isWinningPosition(positionB,4,board,1)){
            winner = 1;
            break;
        }
        //update the move counter
        counter++;
    }
    //print the final board configuration
    printBoard(4,board);
    switch(winner){
        case -1: printf("The game is a draw!\n");
             break;
        case 0: printf("Player 1 has won the game\n");
             break;
        case 1: printf("Player 2 has won the game\n");
            break;

    }
    return 0;
}

Answer 1

General:

I myself am writing a Tic-Tac-Toe game of sorts, so I am go to try to not give my code away too much. For a self-taught beginning C programmer, this code looks good. However, it looks like you were over thinking things a bit based on how long your code is.

---

What you did well on:

• You kept your dependencies at a minimum (for the most part).

• You attempt to use comments to describe what your thought process is and how the program works.

• You used typedef with your struct properly.

• You used a switch instead of multiple if-else test conditions (very good!).

---

Things you could improve on:

• You unnecessarily #include <stddef.h>.

• You aren't using the characters that correspond with a Tic-Tac-Toe game.

     char posChar = (player == 0)? 'X':'Y';
  

  I don't see any reason to not use the X's and O's. We want to be as user-experience friendly as possible to lowering the initial learning curve for first time users.

  char posChar = (player == 0) ? 'X' : 'O';
  

• Your name of the typedef struct is not how the usual programmer would name it.

     typedef struct {
         int xpos;
         int ypos;
     } BRDPOS;
  

  Right now you are using the naming standards that coincide with naming a macro. Here is how I would name a similar struct.

  typedef struct
  {
      int xPos;
      int yPos;
  } BoardPosition;
  

• You have some unused variables: posChar, yStatus, and player. Get rid of them.

• I would #define your board size at the top.

  #define ROWS 4
  #define COLS 4
  

• You always set some variables in a function to your struct values.

   int xPos = position.xpos;
   int yPos = position.ypos;
  

  I'm not seeing the purpose of using these local variables at all. I would remove them and use the direct struct values in their place.

• Initialize variables in your for loop, not outside of them.^(C99)

  for (int row = 0; row < ROWS; row++)
  

• Your comments are sporadic. Also, some areas have too many comments.

     //Ask position to place the X for player 1
     //getPosition keeps asking for the position until a legal position has been entered
     printf("Enter positions for PlayerA:\n");
     BRDPOS positionA = getPosition(4,board);
  

  Here is an example of where I think you are saying too much with your comments. Some code can speak for itself. However, in other areas you have no comments at all.

     return xstatus|ystatus|majdiag|mindiag;
  

  Some people may not know what the bitwise operator | does. You should comment more in areas like these that may be more confusing to newer developers.

• You could use the function puts() in place of your printf().

     printf("Enter position for PlayerB:\n");
  

  Using puts() is a bit better in my mind. Then you don't have to worry about the \n character at the end.

  puts("Enter position for PlayerB: ");
  

Answer 2

I see no major memory-handling or parameter-passing errors, which is pretty good for a self-taught C novice.

---

First, let the compiler do the reviewing…

$ clang -Wall -g -o tictactoe cr41304.c 
cr41304.c:47:10: warning: unused variable 'posChar' [-Wunused-variable]
    char posChar = (player == 0)? 'X':'Y';
         ^
cr41304.c:50:10: warning: unused variable 'yStatus' [-Wunused-variable]
    char yStatus = brd[0][0];
         ^
cr41304.c:103:9: warning: unused variable 'player' [-Wunused-variable]
    int player = 0;
        ^
3 warnings generated.

yStatus is particularly bad: having variables named yStatus and ystatus in the same function is confusing.

---

The user experience could use some polishing.

• The first column is inexplicably wider than the others.
• Your terminology is inconsistent: "PlayerA"/"PlayerB" when prompting vs. "Player 1"/"Player 2" when announcing the winner. (In your code, you also call them players 0 and 1.) I suggest "Player X" and "Player O".

• This statement appears to be left over from development:

  printf("Marking player A as the winner \n");
  

• "Row" and "column" would be clearer than "x-position" and "y-position".
• Zero-based numbering is unusual. To eliminate ambiguity, label the rows and columns when printing the board.

---

You fail to check the return value from scanf() for EOF. If EOF is encountered, it goes into an infinite loop.

---

Your main loop contains copy-and-pasted code for each player. You should be able to restructure your code to avoid repeating yourself.

Your functions all take their parameters in a different order. Try to be consistent.

updateBoard() could be shorter.

void updateBoard(int brd_size, char board[][brd_size], BRDPOS pos, int player) {
    assert(player == 0 || player == 1);
    board[pos.xpos][pos.ypos] = "XO"[player];
}

isWinningPosition() could also be shorter. Avoid using so many variables. Declare your dummy variables in a tighter scope (which you can do since you're using C99). Return early as soon as you find a win. I believe your //Check if the horizontal has the same characters comment is wrong as well — that loop is actually checking for a vertical win.

int isWinningPosition(BRDPOS position,int brd_size,char brd[][brd_size],int player){
    int xPos = position.xpos;
    int yPos = position.ypos;

    // Check for horizontal win
    int win = 1;
    for (int y = 0; win && (y < brd_size); y++) {
        if (brd[xPos][yPos] != brd[xPos][y]) {
            win = 0;
        }
    }
    if (win) return 1;

    // Check for vertical win
    win = 1;
    for (int x = 0; win && (x < brd_size); x++) {
        if (brd[xPos][yPos] != brd[x][yPos]) {
            win = 0;
        }
    }
    if (win) return 1;

    // Check if the major diagonal has the same characters
    win = (xPos == yPos);
    for (int x = 0; win && (x < brd_size); x++) {
        if (brd[xPos][yPos] != brd[x][x]){
            win = 0;
        }
    }
    if (win) return 1;

    // Check if the minor diagonal has the same characters
    win = (xPos == (brd_size - 1 - yPos));
    for (int x = 0; win && (x < brd_size); x++) {
        if (brd[xPos][yPos] != brd[x][(brd_size-1) - x]){
            win = 0;
        }
    }
    return win;
}

Answer 3

I guess you could change all this code:

  while((winner == -1) && (counter < 16)){
        //Print the tic-tac-toe board
        printBoard(4,board);
        //Ask position to place the X for player 1
        //getPosition keeps asking for the position until a legal position has been entered
        printf("Enter positions for PlayerA:\n");
        BRDPOS positionA = getPosition(4,board);
        //Update board with positionA
        printf("Updating the board:\n");
        updateBoard(4,board,positionA,0);
        //Check if the updated position is a winning position
        if(isWinningPosition(positionA,4,board,0)){
            //Mark player A as the winner
            printf("Marking player A as the winner \n");
            winner = 0;
            break;
        }
        //update the move counter
        counter++;
        //print updated board
        printBoard(4,board);
        //Ask position to place the 'O' for player 2
        printf("Enter position for PlayerB:\n");
        BRDPOS positionB = getPosition(4,board);
        //Update board with positionB
        updateBoard(4,board,positionB,1);
        //Check if the updated position is a winning position       
        if(isWinningPosition(positionB,4,board,1)){
            winner = 1;
            break;
        }
        //update the move counter
        counter++;
    }

to only this:

  while((winner == -1) && (counter < 16)){
        //Print the tic-tac-toe board
        printBoard(4,board);
        //Ask position to place the X for player 1
        //getPosition keeps asking for the position until a legal position has been entered
        printf("Enter positions for PlayerA:\n");
        BRDPOS position = getPosition(4,board);
        //Update board with positionA
        printf("Updating the board:\n");
        updateBoard(4,board,position,counter%2);// change
        //Check if the updated position is a winning position
        if(isWinningPosition(position,4,board,counter%2)){// change
            winner = counter%2;// change
            break;
        }

        counter++;
    }

the changes occur due to using: counter%2

In the initial code, you had two very similar pieces of code, with only difference that once you would pass 0 and other times 1 to certain functions depending on who was playing. You were deducing who was playing sort of manually, which is ok, this is a consecutive game - at first one player plays, at the next step another player. But this consecutive nature you can exploit also using the % operator. You already had counter variable. Do it modulo 2, and once you get 1 and other time 0. Now you can use counter%2 instead of manually passing 1 and 0s.

0%2=0
1%2=1
2%2=0
3%2=1
4%2=0
....
etc.