Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
1  
The struct name should only start with an uppercase letter. Having the entire name in caps makes it appear as a macro. –  Jamal Feb 10 at 4:49
    
Does this code work as intended? The board isn't showing up properly for me when I run this. –  syb0rg Feb 10 at 4:51
    
@syb0rg Works for me. –  200_success Feb 10 at 4:58
    
@200_success Odd. I'll work with it anyways I guess :P –  syb0rg Feb 10 at 5:00
    
@syb0rg It crashes here. –  doug65536 Feb 10 at 8:54

3 Answers 3

up vote 14 down vote accepted

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: ");
    
share|improve this answer
    
btw. out of interest, does it make sense to play this kind of 4x4 tic tac toe game? I think this is very easy not to let someone win here. (For learning purposes I know it's ok, I am asking for the sake of playing) –  user36527 Feb 10 at 21:05
    
@hanalbuseltes I think it makes sense to play, sure. –  syb0rg Feb 10 at 22:35
    
@syb0rg - Thanks for taking the time to provide a detailed review. Learned quite a bit from your and 200_success's review. –  sc_ray Feb 11 at 7:00

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;
}
share|improve this answer
    
Thanks for the answer.Learned a lot from your response. –  sc_ray Feb 11 at 7:02

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.
share|improve this answer
    
Could you explain why you would change it? Show the OP your changes. –  syb0rg Feb 10 at 20:34
    
@syb0rg: I mentioned where, I just don't know how to format in code so that it is more visible... as to why,I guess less code/redundancy –  user36527 Feb 10 at 20:48
    
Don't explain it inside of your code block, explain it outside of your code block. Look to 200_success's answer or my own answer as an example. If you can explain your changes, I will give you an upvote. –  syb0rg Feb 10 at 20:50
    
Ahh, much better. Upvote given. –  syb0rg Feb 11 at 0:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.