1
\$\begingroup\$

My first program using C. I would appreciate pointers on how to improve the code. I exit just before turns reach 9 and the grid is filled because it causes all sorts of bugs. The computer is random.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

// Struct with all game state variables.

struct game_data {
    int win;
    int turn;
    int grid[3][3];
} 

game= {
        0,
        1,
        { { 8, 8, 8 },
          { 8, 8, 8 },
          { 8, 8, 8 } }
      };

void player_one_move(struct game_data* game)
{
    int y_val, x_val;
    printf("You are '-1's. Please input co-ordinates in the form 'row  column' for the 3x3 grid:\n");
    scanf(" %d %d", &y_val, &x_val); //Passes player input to variables x_val and y_val

    //Stops illegal moves and places player's position.
    if (game->grid[y_val - 1][x_val - 1] == 8) {
        game->grid[y_val - 1][x_val - 1] = -1;
        printf("\nYour turn:\n\n");
    }
    else {
        player_one_move(game);
    }
}



//Player two function.

/*void player_two_move(struct game_data* game)
{
    int y_val, x_val;
    printf("Please input co-ordinates in the form 'row column' for the 3x3 grid\n");
    scanf(" %d %d", &y_val, &x_val);
    printf("\nYour turn:\n\n");
    game->grid[y_val-1][x_val-1] = 1;
}  */

void computer_move(struct game_data* game)
{
    int x_val = rand() / (RAND_MAX / 4);
    int y_val = rand() / (RAND_MAX / 4);

    if (game->grid[y_val][x_val] == 8) {
        game->grid[y_val][x_val] = 1;
        printf("\nComputer turn:\n\n");
    }
    else {
        computer_move(game);
    }
}

void update(struct game_data* game)
{
    /*for (int y_val = 0; y_val < 3; y_val++) {
        for (int x_val = 0; x_val < 3; x_val++) {
            printf("%d  ", game->grid[y_val][x_val]);            
        }
        printf("\n");   
    }
    printf("\n");*/

    //Displays grid.
    printf("%d | %d | %d \n---+---+---\n %d | %d | %d \n---+---+---\n %d | %d | %d \n\n",
        game->grid[0][0], game->grid[0][1], game->grid[0][2],
        game->grid[1][0], game->grid[1][1], game->grid[1][2],
        game->grid[2][0], game->grid[2][1], game->grid[2][2]);
}

void game_event_won(struct game_data* game)
{
    int left_diag_sum = 0;
    int right_diag_sum = 0;
    int col_sum = 0;
    int row_sum = 0;

    // Counts all columns and rows to find sum
    for (int y_val = 0; y_val < 3; y_val++) {
        for (int x_val = 0; x_val < 3; x_val++) {
            col_sum += game->grid[y_val][x_val];
            row_sum += game->grid[x_val][y_val];
            if (col_sum == -3 || row_sum == -3) {
                game->win = 1;
                printf("You have won.\n");
            }
            if (col_sum == 3 || row_sum == 3) {
                game->win = 1;
                printf("You have lost.\n");
            }
        }
    }

    // Sums diagonals
    for (int y_val = 0; y_val < 3; y_val++)

    {
        left_diag_sum += game->grid[y_val][y_val];
        right_diag_sum += game->grid[y_val][2 - y_val];

        if (left_diag_sum == -3 || right_diag_sum == -3) {
            game->win = 1;
            printf("You have won.\n");
        }
        if (left_diag_sum == 3 || right_diag_sum == 3) {
            game->win = 1;
            printf("You have lost.\n");
        }
     }
}

int main(void)
{
    //Initialises random number generator.
    srand((unsigned)time(0));


    while (game.win == 0 && game.turn < 9) {
        if (game.turn % 2) {
            player_one_move(&game);
            game.turn++;
        }

        else {
            //player_two_move(&game); Player two function
            computer_move(&game);
            game.turn++;
        }
        update(&game);
        game_event_won(&game);
    }

    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

Before I even looked at your program, I though I'd let the compiler discover any obvious flaws by compiling it with these flags:

gcc -Wall -Wextra -pedantic -O2 noughts-and-crosses.c

I expected a few warnings, but there weren't any. This means your code is already free from the worst mistakes. This shows that you have already put some good work into it, and that the code is ready to be inspected by human proofreaders. Very good.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

The headers are in alphabetical order. This is good. Since there are only 3 of them I cannot say whether it is coincidence or arranged by your IDE or done by you manually. Anyway, this is how professional programs look. (Except when the headers do need a certain order. Then this order is of course more important than alphabetical.)

// Struct with all game state variables.

struct game_data {
    int win;
    int turn;
    int grid[3][3];
}

You should remove the empty line between the comment and the beginning of the struct. As it is now, the comment reads like a general remark that applies to all the part below it and not just the struct.

This struct definition is the best place to document which values are valid for the win fields. There are several possible choices:

  • true, false
  • 0, 1
  • 0, 1, 2
  • -1, 0, -1
  • 0, 'o', 'x'
  • 0, '0', '1'
  • 0, '1', '2'

It's good style to avoid this possible confusion by commenting the possible values. Especially when you use int as the data type, since that data type is used for almost everything.

For the turn field, I first thought it would mark the player whose turn it is. But that's not what the code says. It's actually the number of turns that have already been played. Therefore I'd expect it to be called turns instead of turn.

The grid field is obvious since it is a 3 by 3 array, which for noughts and crosses can only mean the content of the board. There should be a comment that explains the possible values. Again, there are almost as many possibilities as for the win field.

game= {
        0,
        1,
        { { 8, 8, 8 },
          { 8, 8, 8 },
          { 8, 8, 8 } }
      };

You surprised me a lot with this part. I first thought about a syntax error, but then I saw that you left out the semicolon after the struct definition. This is unusual since an empty line typically means that the two parts around the empty line are somewhat independent. This is not the case here.

The usual form is to put the semicolon at the end of the struct definition and then repeat the words struct game_data, so that the full variable declaration starts with struct game_data game = {.

void player_one_move(struct game_data* game)
{
    int y_val, x_val;
    printf("You are '-1's. Please input co-ordinates in the form 'row  column' for the 3x3 grid:\n");
    scanf(" %d %d", &y_val, &x_val); //Passes player input to variables x_val and y_val

    //Stops illegal moves and places player's position.
    if (game->grid[y_val - 1][x_val - 1] == 8) {
        game->grid[y_val - 1][x_val - 1] = -1;
        printf("\nYour turn:\n\n");
    }
    else {
        player_one_move(game);
    }
}

When the game starts, the empty board is not printed. Therefore there is absolutely no clue that the coordinates are in the range 1..3. It would be far easier if there were some example coordinates written somewhere.

Using 8 for an empty cell is something I don't understand. An 8 does not look like empty at all. A much better choice would be an actual space or at least an underscore or dot.

Also, having -1 for one player and 1 for the other leads to a board layout in which the position of the vertical lines depends on which player plays where. This has nothing to do with the game in reality, where the vertical and horizontal lines are fixed during a game.

void computer_move(struct game_data* game)
{
    int x_val = rand() / (RAND_MAX / 4);
    int y_val = rand() / (RAND_MAX / 4);

This again looks unusual. First, why do you divide by 4 instead of by 3? This gives you random numbers between 0 and 3, therefore it might happen that the computer plays off the board (if the memory right behind the struct game_state just happens to have an 8 stored there).

Second, the usual pattern for generating a random number between 0 and n is to just calculate rand() % n, which in this case is rand() % 3.

    if (game->grid[y_val][x_val] == 8) {
        game->grid[y_val][x_val] = 1;
        printf("\nComputer turn:\n\n");
    }
    else {
        computer_move(game);
    }
}

void update(struct game_data* game)
{
    //Displays grid.
    printf("%d | %d | %d \n---+---+---\n %d | %d | %d \n---+---+---\n %d | %d | %d \n\n",
        game->grid[0][0], game->grid[0][1], game->grid[0][2],
        game->grid[1][0], game->grid[1][1], game->grid[1][2],
        game->grid[2][0], game->grid[2][1], game->grid[2][2]);
}

The above code looks quite nice since it visually tells the reader that it prints the 3x3 board. You could make it even nicer if you'd split the string after each \n, like this:

    printf(
        "%d | %d | %d \n"
        "---+---+---\n"
        " %d | %d | %d \n"
        "---+---+---\n"
        " %d | %d | %d \n"
        "\n",
        game->grid[0][0], game->grid[0][1], game->grid[0][2],
        game->grid[1][0], game->grid[1][1], game->grid[1][2],
        game->grid[2][0], game->grid[2][1], game->grid[2][2]);

Now the code looks almost exactly how the board will be printed, which is good.

void game_event_won(struct game_data* game)
{
    int left_diag_sum = 0;
    int right_diag_sum = 0;
    int col_sum = 0;
    int row_sum = 0;

    // Counts all columns and rows to find sum
    for (int y_val = 0; y_val < 3; y_val++) {
        for (int x_val = 0; x_val < 3; x_val++) {
            col_sum += game->grid[y_val][x_val];
            row_sum += game->grid[x_val][y_val];
            if (col_sum == -3 || row_sum == -3) {
                game->win = 1;
                printf("You have won.\n");
            }
            if (col_sum == 3 || row_sum == 3) {
                game->win = 1;
                printf("You have lost.\n");
            }
        }
    }

    // Sums diagonals
    for (int y_val = 0; y_val < 3; y_val++)

    {
        left_diag_sum += game->grid[y_val][y_val];
        right_diag_sum += game->grid[y_val][2 - y_val];

        if (left_diag_sum == -3 || right_diag_sum == -3) {
            game->win = 1;
            printf("You have won.\n");
        }
        if (left_diag_sum == 3 || right_diag_sum == 3) {
            game->win = 1;
            printf("You have lost.\n");
        }
     }
}

This function is the most important and the most complicated at the same time, which already sounds bad. It is also full of bugs.

For example, it doesn't recognize it when I play at 2 1, 2 2, 2 3. That should be a win for me, but it isn't. The reason for this is that you also add each 8 (which means empty) to the sum. Therefore, the only situations in which I can currently win are the horizontal 1 1, 1 2, 1 3 or the vertical 1 1, 2 1, 3 1 (but only if the computer and I have also filled the cells at 1 2, 1 3, 2 2 and 2 3.

Just counting up to 3 or down to -3 isn't enough. For example, the combination 1 3, 2 1, 2 2 is not a winning combination, but might be counted as such by your code.

A better approach is to look at each possible combination (3 horizontal, 3 vertical, 2 diagonal) and check for each one separately and independently whether all its cells have the same value and at least one of these cells is not empty.

One time I played against the computer, and to be sure I could not win, I played at 1 1, 1 2, 2 1, 2 2. The computer meanwhile got 3 in a row, but nevertheless the game said You have won., which is wrong.

This function can also print You have won. twice for the same situation (once horizontally or vertically, and once more diagonally). This is another bug.

int main(void)
{
    //Initialises random number generator.
    srand((unsigned)time(0));


    while (game.win == 0 && game.turn < 9) {
        if (game.turn % 2) {
            player_one_move(&game);
            game.turn++;
        }

        else {
            //player_two_move(&game); Player two function
            computer_move(&game);
            game.turn++;
        }
        update(&game);
        game_event_won(&game);
    }

    return 0;
}

There are some more things to how you should structure the code of the game, but the first thing to do is to fix the bugs. After that, you are welcome to post a follow-up question with the fixed code.

\$\endgroup\$

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.