17
\$\begingroup\$

I recently started learning C, and this is my first fairly large (to me) program. It's a basic Tic Tac Toe game for the console. There's no AI, it's just a 2-player game. Is there anything I can improve?

Some problems I've been told elsewhere:

  • There's no case for tying/drawing.

  • I should split the place function into two.

I also think it would be better to use global variables for board and player, since they're used so often.

// Tic Tac Toe in C. V1.0 by James 26/05/2019

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

#define SIZE 3

// Draws the board along with rows/cols, numbered.
void draw_board(char board[])
{
    system("clear");
    printf("# 1 2 3\n");

    // Rows
    for (int i = 0, n = 0; i < SIZE; i++)
    {
        // Columns
        printf("%d ", i + 1);
        for (int j = 0; j < SIZE; j++)
        {
            printf("%c ", board[n]);
            n++;
        }
        printf("\n");
    }
}

// Initializes board to '-' characters.
void init_board(char board[])
{
    for (int i = 0; i <  SIZE * SIZE; i++)
    {
        board[i] = '-';
    }
}

// Returns true if the piece was successfully placed,
// false if the position was invalid or already taken.
bool place(char board[], char player)
{
    char posinput[64];

    printf("%c, pick your position (xy, rc): ", player);
    scanf("%s", posinput); // <-- I realise that this is potentially bad, but realistically, 
                           // the user would have to be trying to break something, and
                           // this is just a little program I made for practice.

    int row = (posinput[0] - '0') - 1;
    int col = (posinput[1] - '0') - 1;

    int pos = col + row * SIZE;

    if (pos >= 0 && pos < SIZE * SIZE)
    {
        if (board[pos] == 'x' || board[pos] == 'o')
            return false;

        board[pos] = player;
        return true;
    }

    return false;
}

// Returns true if there are three of the same chars in a row.
// b = board, p = player. Shortened for readability.
bool check(char b[], char p) 
{
    // Check rows
    if (b[0] == p && b[1] == p && b[2] == p)
        return true;
    if (b[3] == p && b[4] == p && b[5] == p)
        return true;
    if (b[6] == p && b[7] == p && b[8] == p)
        return true;


    // Check columns
    if (b[0] == p && b[3] == p && b[6] == p)
        return true;
    if (b[1] == p && b[4] == p && b[7] == p)
        return true;
    if (b[2] == p && b[5] == p && b[8] == p)
        return true;


    // Check diagonals
    if (b[0] == p && b[4] == p && b[8] == p)
        return true;
    if (b[2] == p && b[4] == p && b[6] == p)
        return true;

    // If no one won, return false
    return false;
}

int main(void)
{
    char board[SIZE * SIZE];
    char player = 'x';
    init_board(board);

    while (true)
    {
        draw_board(board);

        if (place(board, player))
        {
            if (check(board, player))
                break;

            if (player == 'x')
                player = 'o';
            else
                player = 'x';
        }
    }

    draw_board(board);
    printf("-----------------------------\n");
    printf("Player %c wins!!!\n", player);
    printf("-----------------------------\n");
}
\$\endgroup\$
  • 6
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question linking back to this one instead, but it's advised to wait at least a day before posting a follow-up. \$\endgroup\$ – Mast May 26 at 13:11
  • \$\begingroup\$ Mod note: If you want to critique an aspect of the code as it is presented here, please write an answer. If you want to argue about the peculiarities of C and your preferences in dealing with forward declarations, please use Code Review Chat. Comments are not the place for either of these things. Thanks :) \$\endgroup\$ – Vogel612 May 29 at 9:24
18
\$\begingroup\$

First of all: nice work! It's easy to read and understand.

Program organization

It's very good that you split the task to small functions. Reading the body of main reveals nicely the overall flow.

Ideas for further improvement:

  • place does two things: it reads input from user and updates the state of the board. It would be good to separate these logically distinct steps to different functions. A new function, say read_next_move could return boolean just like place, and take parameters board and pointers to x and y, whose values will be used when updating the board state.

  • The name check is too generic. How about is_game_over or has_player_won.

  • Printing the winner would be good to move to a dedicated function, as you did with other steps.

Use more constants

The special symbols -, x and o would be good to define as constants. So that instead of if (board[pos] == 'x' || board[pos] == 'o'), you could write more descriptively if (board[pos] == PLAYER_X || board[pos] == PLAYER_O), and if ever needed change the values easily. By the way, in this particular example I would replace the condition with if (board[pos] != AVAILABLE).

\$\endgroup\$
  • \$\begingroup\$ Thanks, I actually split up place into 2 functions, but I wasn't allowed to add a new version of the code to this post. I made a function called get_pos that gets input for the coord, then calculates the position in the array, and returns it, place uses that function, and tries to place, checking if it can. I also renamed check to check_win, and also have a function, check_draw. I'll also use constants for the player pieces, thanks! \$\endgroup\$ – J. Czekaj May 27 at 12:41
  • \$\begingroup\$ @J.Czekaj: Yup, instead of returning both x and y separately, it makes more sense for a get_move function to handle encoding them into an array index like the rest of the program uses. (Or better, have your players enter moves using the number pad in a way that matches the TTT grid layout, so it's just a single 1-9 digit: subtract '1' from it to get a 0..8 index). You might handle the illegal-move / try-again prompting inside that one function, so it only returns an error on I/O error or something. \$\endgroup\$ – Peter Cordes May 28 at 5:25
10
\$\begingroup\$

There's two things I'd change to simplify your code:

  • Use a two dimensional array. Sure you can do the conversion from a 2d position to 1d easily, but the compiler can do and board[1][1] is rather more obviously the middle of the board than board[4].
  • Instead of hardcoding your logic for what positions you have to check, think about a programmatic approach using a loop. This not only avoids duplicating lots of code, but also makes it trivial to extend your tic-tac-toe game to a larger board.
\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Vogel612 May 29 at 9:25
9
\$\begingroup\$

I think youre code is quite nice. Easy to read and the tasks are nicely splitted into functions. So theres not much to add.

Theres just one thing bothering me.

Don't omit curly braces. This:

// Check rows
if (b[0] == p && b[1] == p && b[2] == p)
    return true;
if (b[3] == p && b[4] == p && b[5] == p)
    return true;
if (b[6] == p && b[7] == p && b[8] == p)
    return true;

Should be this:

// Check rows
if (b[0] == p && b[1] == p && b[2] == p) {
    return true;
}
if (b[3] == p && b[4] == p && b[5] == p) {
    return true;
}
if (b[6] == p && b[7] == p && b[8] == p) {
    return true;
}

You wonder why? It can lead to bugs. One example is apple's SSL bug

For more discussion see: https://stackoverflow.com/questions/359732/why-is-it-considered-a-bad-practice-to-omit-curly-braces

\$\endgroup\$
  • 2
    \$\begingroup\$ Thanks, for the case I'm using it in, I think it's okay. If I added curly braces, it would just add extra clutter, since there's so many if's. If it was one single if, then it would make sense, but when they're all lined up like that, it's clear to see if you made a mistake. \$\endgroup\$ – J. Czekaj May 26 at 11:34
  • 1
    \$\begingroup\$ @J.Czekaj: another alternative is to make one big if with a multi-line condition. It's non-terrible if you format carefully, with || at the end of each line. But yes I'd agree with bending that style guideline for this function where there are many similar conditions to check, and that's all you're doing. \$\endgroup\$ – Peter Cordes May 26 at 17:26
  • 1
    \$\begingroup\$ @J.Czekaj: But note that "many similar" is a red flag that maybe you should write a loop or helper function instead. Or take advantage of an early-out, e.g. only check the diagonals and central + squares inside if(b[4] == p) { ... /* middle square */ }. It's a tradeoff between more complex logic for a human programmer to verify (compiler hopefully spots it either way) vs. simpler but larger code with those fully unrolled checks of all 3 rows / cols. \$\endgroup\$ – Peter Cordes May 26 at 17:30
  • 1
    \$\begingroup\$ If you let your IDE format the code automatically and compile the code with -Wmisleading-indentation, it's perfectly ok to omit the curly braces. \$\endgroup\$ – Roland Illig May 26 at 18:34
  • 2
    \$\begingroup\$ Let’s please collectively stop using the “goto fail” bug as an argument to justify our preferences, and let’s pretend that the bug was caused by lack of braces rather than by a merge conflict that could also have happened with braces (albeit less likely). In fact, the article you link in your answer (and which you probably didn’t read) also says that the lack of braces isn’t the problem here. \$\endgroup\$ – Konrad Rudolph May 27 at 10:05
7
\$\begingroup\$

The following remarks may be nitpicks. Since your code is already quite good, it's all I have to say. :)

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

Since all the above headers are from the standard C library, they should be in alphabetical order.

When you include other headers like <sys/type.h>, the order is sometimes important. But not in your simple program.

#define SIZE 3

Having this constant means that anyone else may later change the 3 into a 5 and can expect that the program still works reasonably well. If your code doesn't provide this guarantee, you should add a small comment explaining that this constant should not be modified.

// Draws the board along with rows/cols, numbered.
void draw_board(char board[])

Function signatures in C should not contain arrays since these behave surprisingly in several cases (like multi-dimensional arrays). Also, it sounds strange to say "to draw the board, given a character array". The function signature would sound a lot better like this:

void draw_board(tic_tac_toe_board *board)

This changes the wording to "draw the board, given a board", which is a bit redundant, but it focuses on the problem domain instead of the technical level, which makes the code easier to understand, especially if you want to explain programming to laymen.

To make the above function signature valid, you need to declare the tic_tac_toe_board as a type:

typedef struct {
    char cell[SIZE * SIZE];
} tic_tac_toe_board;

With this type definition, your code may look like this:

void init_board(tic_tac_toe_board *board)
{
    for (int i = 0; i <  SIZE * SIZE; i++)
        board->cell[i] = '-';
}

It's a bit more effort to write board->cell[i] instead of the simple board[i] from before, but it's worth it since you can now talk about a tic-tac-toe board, without having to mention that it is implemented as a character array.

Having this abstraction also means that you can easily extend the board by recording the history of moves, just in case you want to implement an undo feature later:

typedef struct {
    char cell[SIZE * SIZE];
    int moves_count;
    int moves[SIZE * SIZE];
} tic_tac_toe_board;

Next topic, the communication with the user:

    printf("%c, pick your position (xy, rc): ", player);

That message is a mistake: xy is not the same as rc since r corresponds to y, not x. It should be either (xy, cr) or (yx, rc).

    scanf("%s", posinput);

Instead of reading a string here, there's a completely different idea. Most keyboards have a numeric block, which by coincidence consists of 3×3 keys. When you map the keyboard layout as follows, the human players just need a single key instead of two to enter a coordinate, plus the position on the keyboard matches exactly the position on the board:

7 8 9
4 5 6
1 2 3

It only works for 3×3 though. But if you accept that limitation:

int pos;
if (scanf("%d", &pos) == 1) {
    int row = (SIZE - 1) - (pos - 1) / SIZE;
    int col = (pos - 1) % SIZE;
    int board_pos = col + row * SIZE;
}

Going further in your code:

if (board[pos] == 'x' || board[pos] == 'o')
    return false;

I would rather say board[pos] != '-', for 3 reasons: it is shorter, it is faster, and when you add a third player someday the code is still correct.

// Returns true if there are three of the same chars in a row.
// b = board, p = player. Shortened for readability.

A very nice comment. Short, to the point, and informative.

The rest of the code looks good already. There's just one missing thing. Anecdotal evidence suggests that in the game of tic-tac-toe, players familiar with each other will tie 100% of the time due to the limited number of outcomes. I suggest you implement the tie rule. It's very simple: if SIZE * SIZE moves have been played and there is still no winner, it's a tie.

\$\endgroup\$
  • 1
    \$\begingroup\$ The history should not be part of the ttt_board struct. When you add an AI that recursively tries moves to find forced wins and avoid forced losses, it's going to be passing around copies of boards with a move made, and the history is in the recursion stack frames (local vars). Making the board struct 4x larger (on most C implementations) with an int array sounds terrible. \$\endgroup\$ – Peter Cordes May 28 at 5:13
  • \$\begingroup\$ @Peter that's a good argument, I agree fully with it. It was just the first thing that came to my mind when I wrote the answer and wanted to show how easily a struct can be extended. Maybe it would have been a better idea to add a field char turn that tells whose turn it is. \$\endgroup\$ – Roland Illig May 28 at 6:36
  • \$\begingroup\$ Maybe, but that's redundant for some cases where you want to use a ttt_board. You can nest structs, though, for a complete game-position struct that includes the turn. \$\endgroup\$ – Peter Cordes May 28 at 7:15
6
\$\begingroup\$

The code looks clean and readable.

You could rename check to checkForWin to tell what it checks.

I would also suggest to improve that procedure by declaring the lines as a constant array instad of writing a similar test 8 times:

const int lines[8][3] = {
    { 0, 1, 2 }, // rows
    { 3, 4, 5 },
    { 6, 7, 8 },
    { 0, 3, 6 }, // columns
    { 1, 4, 7 },
    { 2, 5, 8 },
    { 0, 4, 8 }, // diagonals
    { 2, 4, 6 }
};

bool checkForWin(char b[], char p) 
{
    for (int i = 0 ; i < 8 ; i++ ){
        if (b[lines[i][0]] == p && b[lines[i][1]] == p && b[lines[i][2]] == p){
            return true;
        }
    }
    // No winnig line found
    return false;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Neither of those two options are very scalable (it's nice to declare a SIZE constant, less nice if you can't change it anyhow with adapting the logic). The logic is very easy to implement for arrays of every size - check all rows, all columns and the two diagonals. No need to hardcode it. \$\endgroup\$ – Voo May 26 at 17:47
  • \$\begingroup\$ Yes, the SIZE problem needs to be addressed. If you want to honor the SIZE constant, you can generate the array programmatically (and introduce variables for the array sizes). It is clean to define the topology and then use it to drive the program. For instance, if the next step is to have the computer play, and it has to find opportunities for double-threats, it will be much simpler with such a lines array. If you change for a 3x3x3 board, the code doesn't change. There are just more lines. \$\endgroup\$ – Florian F May 26 at 18:20
  • \$\begingroup\$ But as soon as you generate the lines array programmatically, you already have the code to do the check directly (since that's basically what you're doing when creating the array). Just seems rather roundabout. \$\endgroup\$ – Voo May 26 at 20:18
  • 1
    \$\begingroup\$ It is very enlightening to see that you can abstract the board as a generic set of cells joined by a set of lines, to see that you can define the shape of the board in one place and program the gameplay regardless of that shape. It is called separation of concerns. \$\endgroup\$ – Florian F May 26 at 21:02
  • 4
    \$\begingroup\$ Hard coding those “lines” into rows columns and diagonals feels odd. Not very human readable, and at the same time not dynamic to board size. So kind of a lose lose. \$\endgroup\$ – Cooper Buckingham May 27 at 2:27
5
\$\begingroup\$

Starting from the top, defining the SIZE macro is good - this potentially allows you to change the size of the board by modifying one number. It is especially good because it allows you to later come back and change it to the name of a global variable instead of a constant number, which then allows you to define the size of the board runtime.

You then throw away some of that utility in draw_board(), by printing the column numbers as a constant ("# 1 2 3\n"), rather than by using the nice macro you defined just a few lines prior. You should print the column numbers with a loop, just like you print the board itself. Consider also writing ones for the symbols (i.e. EMPTY, PLAYER1 and PLAYER2) so you can change them if desired.

init_board() is great, but you could use memset(board, '-', SIZE * SIZE) instead of the loop.

In place(), your scanf() format should not be "%s". You're expecting two numbers (or digits), so scan for those: "%d%d" or "%c%c" will do the job and allow for improvements later down the line. In addition, you can check for whether there already is a symbol at the coordinate in a single comparison with board[pos] != '-'.

Also, you should return the index on success and a faulty value like -1 on failure, so that your check() can take that as input and only check the rows, columns and diagonals that the token is a part of. Something like this:

// In the main loop:
int index = place();
if (index >= 0) 
{
    if (check(board, index, player))
        break; // Placement succeeded and resulted in a win, break loop.
    /* … */
}

Next, in check(), you check a 3x3 board, completely ignoring that nice SIZE macro you defined. You should instead use a loop to check because then you can freely change the SIZE of the board later down the line. Combined with the index for input, you'd have something like...

bool check(char board[], int index, char player)
{
    int row = index / SIZE;
    int col = index % SIZE;

    // We'll unconditionally check the diagonals too.
    int rowCount = 0;
    int colCount = 0;
    int diagLCount = 0; // Top-left diagonal
    int diagRCount = 0; // Top-right diagonal

    for (int i = 0; i < SIZE; i++) {
        // Comparisons (< > >= <= != ==) evaluate to a 1 (true) or a 0 (false),
        // so we can just sum them into the counters.
        rowCount += board[row * SIZE + i] == player;
        colCount += board[i * SIZE + col] == player;
        diagLCount += board[i * SIZE + i] == player;
        diagRCount += board[i * SIZE + SIZE - i] == player;
    }

    // Just unconditionally return whether any of the counters reached the desired value.
    return rowCount == SIZE || colCount == SIZE || diagLCount == SIZE || diagRCount == SIZE;
}

Finally, you may want to define a function-like macro or a function for indexing the board to reduce the chance of having a typo cause a bug and to make it somewhat more legible. It can also reduce the amount of work you need to put in if you choose to change the indexing later on.

#define AT(X, Y)   [(Y) * SIZE + (X)]
int at(int x, int y) { return y * SIZE + x; }
// which allow you to write something like...
if ( board AT(x, y) == '-' )
   board[at(x, y)] = 'x';
\$\endgroup\$
  • 3
    \$\begingroup\$ Using a macro for something that can easily be done by a function is rather awful. \$\endgroup\$ – Voo May 26 at 18:38
  • \$\begingroup\$ @Voo Uh... What do you think macros are useful for? \$\endgroup\$ – user175869 May 26 at 18:52
  • 4
    \$\begingroup\$ Macros are for things that cannot be done by inline functions. Which, yes, means they should be used as rarely as possible. \$\endgroup\$ – Cody Gray May 26 at 19:58
  • 1
    \$\begingroup\$ macros dont follow the rules of the language. they are maintenance hazards and introduve hard to spot bugs. besides using them for constants in c you should rarely use them... \$\endgroup\$ – Sandro4912 May 27 at 3:53
  • 1
    \$\begingroup\$ @PeterCordes It will also match a leading - if there is one, but reading input is a beast of its own... \$\endgroup\$ – user175869 May 28 at 15:04
2
\$\begingroup\$

Overall, this is well-written code. One thing I would change besides what has already been suggested by others is your call to system("clear") which creates an OS-specific dependency.

You can instead use #ifdef to check the platform and then call the appropriate command to clear the screen. Additionally, this can be wrapped in a function to allow for code that is easy to reuse.

Example:

void clear() {
    #ifdef _WIN32
        system("cls");
    #else
        system("clear");
    #endif
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks, I was thinking about adding something like this, but I just ended up forgetting about it. I didn't think it was that simple! \$\endgroup\$ – J. Czekaj May 29 at 11:09

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.