Simple Tic Tac Toe game inside terminal

Question

Hello fellow programmers. I started learning C last week and today I wanted to see if I can make a working program, so I made this game:

/*
 * TESTED IN LINUX - 2020
*/

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

int player = 1, choice;
int places[10] = {'o', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

void switchPlayers();
void displayBoard();
int markBoard(char mark);
int checkForWin();

int main() {


    while (!checkForWin()){
        system("clear");
        displayBoard();
        switchPlayers();
    }
    system("clear");
    displayBoard();
    checkForWin();

    return 0;
}


void switchPlayers(){
    if (player == 1) {
        printf("     Player 1 choose: ");
        scanf("%d", &choice);
        if (markBoard('X'))
            player = 1;
        else
            player = 2;

    }
    else if (player == 2){
        printf("     Player 2 choose: ");
        scanf("%d", &choice);
        if (markBoard('O'))
            player = 2;
        else
            player = 1;

    }
}

void displayBoard(){
    printf("\n             X & O           \n Player 1 (X) - Player 2 (O)\n\n");

    printf("\t    |   |   \n"
           "\t  %c | %c | %c \n"
           "\t ___|___|___\n"
           "\t    |   |   \n"
           "\t  %c | %c | %c \n"
           "\t ___|___|___\n"
           "\t    |   |   \n"
           "\t  %c | %c | %c \n"
           "\t    |   |   \n\n"
           ,  places[1], places[2],
           places[3], places[4], places[5],
           places[6], places[7], places[8], places[9]);
}

int markBoard(char mark){
    for (int i = 1; i < 10; ++i) {
        if (choice == i && places[i]-48 == i) {
            places[i] = mark;
            return 0;
        }
    }
    return 1;
}

int checkForWin() {
    short draw = 0;

    //Horizontal check
    for (int i = 1; i < 10; i += 3) {
        if (places[i] == places[i + 1] && places[i + 1] == places[i + 2]) {
            printf("\tYou won!\n");
            return 1;
        }
    }
    //Vertical check
    for (int i = 1; i < 4; i += 1) {
        if (places[i] == places[i + 3] && places[i + 3] == places[i + 6]) {
            printf("\tYou won!\n");
            return 1;
        }
    }
    //Diagonal check
    if (places[1] == places[5] && places[5] == places[9]) {
        printf("\tYou won!\n");
        return 1;
    } else if (places[3] == places[5] && places[5] == places[7]) {
        printf("\tYou won!\n");
        return 1;
    }
    //Check for draw
    for (int j = 1; j < 10; ++j) {
        if (places[j] - 48 != j)
            draw++;
    }
    if (draw == 9){
        printf("\t  Draw!\n");
        return 1;
    }

    return 0;
}

Do you have any tips on how I could make it more efficient or fix something I did wrong?

Answer 1

On the whole this is nicely done for an application made during your first week of C programming. The functions are generally reasonably-sized, the code is comprehensible and the design decisions are understandable given the size and purpose of the application.

My remarks mostly center around improving the design to support new features and avoid bugs. The merit of many of my points may be negligible if you're never planning on adding to the application beyond what you have, but if you'd like to add (for example) even fairly unassuming improvements like an AI opponent or arbitrary board sizes, the current design would be pushed beyond its narrow limits.

Avoid globals

Your core state variables

int player = 1, choice;
int places[10] = {'o', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

should all be scoped to main at minimum. The problem with the existing design is that all functions operating on these global variables are non-idempotent. This means they are unsafe in a multithreaded environment and modify state outside of themselves which makes the application difficult to reason about and may lead to bugs.

The parameterless functions in your original code creates an illusion of simplicity. But in reality these functions are quite unsafe and would inhibit a larger application significantly.

Encapsulate related data

int player = 1, choice;
int places[10] = {'o', '1', '2', '3', '4', '5', '6', '7', '8', '9'};

Are loose variables that are conceptually attributes of the same entity and as such should be grouped in a TicTacToePosition structure along with a set of functions that operate on this structure. This makes it easier to understand the purpose and relationship between these pieces of data. Taken out of context, a variable name like places has non-obvious purpose, but as a struct member, position.places or ttt_state.squares is a bit clearer.

Separate UI from game logic

If you want to generalize and expand on your code, you'll need to detach user interaction from game logic. Doing so makes the code maintainable and expandable through looser coupling.

There are many subtle manifestations of the strong UI-game logic coupling throughout the app, like:

int markBoard(char mark){
    for (int i = 1; i < 10; ++i) {
        //       ^
        if (choice == i && places[i]-48 == i) {
            //                      ^^^
            places[i] = mark;
            return 0;
        }
    }
    return 1;
}

In the above code, the 1-indexing seems to be a convenience measure to avoid having to normalize 1-indexed user input to internal logic. But this convenience leads to an awkward board design for the programmer and a confusing magic 'o' character at the 0-th index.

Additionally, -48 is a conversion between external UI and internal game logic that markBoard shouldn't be responsible for. The correct name for this function is convertFromUserInputCharAndMarkBoard, which is overburdened. Normalize/sanitize user input outside of the tic tac toe board logic. This lets you keep the interface user-centric while supporting an intuitive internal representation for the programmer.

switchPlayers does more than switch players: it also takes user input. Those are two distinct things that should be separate.

checkForWin does win checking but also does IO, a side effect. Better to just return the result and let the caller handle the IO. In fact, checkForWin is called twice in the main function, once to check for a win and the second time to display the winning player after clearing the screen.

UX

I recommend spelling out the input format more precisely and using X wins! or O wins! instead of You won!.

Instead of Player 1 and Player 2, using X and O throughout removes ambiguity and lets you avoid a layer of indirection by having to display/explain Player 1 (X) - Player 2 (O) which asks the user to mentally translate between multiple terms for the players.

Bad input into scanf spams the console and there are no error messages or handling to speak of. scanf isn't the right tool here; use fgets to pull the line in as a string and parse the number out of it.

I'm not really crazy about system("clear"). It feels invasive. If you're committed to this sort of interface, I'd go all in with curses. Or just keep it simple and keep printing without clearing.

Avoid convoluted logic

In checkForWin, the draw logic is somewhat hard to follow:

//Check for draw
for (int j = 1; j < 10; ++j) {
    if (places[j] - 48 != j)
        draw++;
}
if (draw == 9){
    printf("\t  Draw!\n");
    return 1;
}

Once again, the -48 is an artifact of user input conversion that really doesn't belong in the game engine. Instead of a player variable and this manual draw check logic, most two-player board games use a single number, ply, which counts the turn. A draw check then becomes ply >= length of board assuming is_won is called first, figuring out whose turn it is becomes ply % 2 and switching sides is ply++.

Ply can be used to avoid pointless win checks if not enough moves have been played. In tic tac toe, it seems like a minor optimization but it can speed up an AI that's running win checks thousands of times per turn, and it's a single extra line of code.

Consider breaking commented code out into functions

The checkForWin function has 4 distinct parts to it: checking horizontals, verticals, diagonals and draws. Each could be a separate function instead of delimiting the areas with comments. Otherwise, some of these loops could be merged and the logic simplified (it's debatable which is best).

Code style

• Keep your braces consistent: void switchPlayers(){ should be void switchPlayers() {.

• Use #include <stdbool.h>:

    if (draw == 9){
        printf("\t  Draw!\n");
        return 1;
    }
  

  could then be

    if (draw == 9){
        printf("\t  Draw!\n");
        return true;
    }
  

  which is easier for the programmer to understand.

Possible rewrite

While I'd prefer to use a bitboard and bitmasks to check win patterns, I think it's most instructive to keep the array format to avoid too radical of a departure from your design.

Feel free to accuse this code of being prematurely future-proof or anticipating adding AI and other features. Fair enough--it is more code. While I've gone somewhat deep into generalizing board sizes and so forth, you can cherry-pick the techniques that make sense for you and take the rest with a grain of salt.

Future steps might be adding a GUI, generalizing the board sizes with malloc or a FAM, adding an AI, adding a variant or network play.

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

struct TicTacToePosition {
    uint8_t ply;
    uint8_t board_len;
    uint8_t side_len;
    char board[3][3];
};

struct TicTacToePosition ttt_init() {
    struct TicTacToePosition ttt_pos = {};
    ttt_pos.board_len = sizeof ttt_pos.board;
    ttt_pos.side_len = sizeof ttt_pos.board[0];
    return ttt_pos;
}

char ttt_current_player(const struct TicTacToePosition *pos) {
    return pos->ply % 2 ? 'O' : 'X';
}

char ttt_last_player(const struct TicTacToePosition *pos) {
    return pos->ply % 2 ? 'X' : 'O';
}

bool ttt_is_board_full(const struct TicTacToePosition *pos) {
    return pos->ply >= pos->board_len;
}

bool ttt_legal_move(const struct TicTacToePosition *pos, int row, int col) {
    return row >= 0 && row < pos->side_len && 
           col >= 0 && col < pos->side_len && !pos->board[row][col];
}

bool ttt_try_move(struct TicTacToePosition *pos, int row, int col) {
    if (!ttt_legal_move(pos, row, col)) {
        return false;
    }

    pos->board[row][col] = ttt_current_player(pos);
    pos->ply++;
    return true;
}

bool ttt_line_win(const unsigned int len, const char *arr) {
    for (int i = 1; i < len; i++) {
        if (!arr[0] || !arr[i] || arr[0] != arr[i]) {
            return false;
        }
    }
    
    return true;
}

bool ttt_is_won(const struct TicTacToePosition *pos) {
    if (pos->ply < 5) return false;

    const uint8_t len = pos->side_len;
    char left_diag[len];
    char right_diag[len];

    for (int i = 0; i < len; i++) {
        char column[len];
        left_diag[i] = pos->board[i][i];
        right_diag[i] = pos->board[i][len-i-1];

        for (int j = 0; j < len; j++) {
            column[j] = pos->board[j][i];
        }

        if (ttt_line_win(len, pos->board[i]) || ttt_line_win(len, column)) {
            return true;
        }
    }

    return ttt_line_win(len, left_diag) || ttt_line_win(len, right_diag);
}

char ttt_fmt_square(const struct TicTacToePosition *pos, int i, int j) {
    return pos->board[i][j] ? pos->board[i][j] : i * pos->side_len + j + '1';
}

void ttt_print_board(const struct TicTacToePosition *pos) {
    puts("");

    for (int i = 0; i < pos->side_len; i++) {
        for (int j = 0; j < pos->side_len - 1; j++) {
            printf("   |");
        }

        printf("\n %c ", ttt_fmt_square(pos, i, 0));

        for (int j = 1; j < pos->side_len; j++) {
            printf("| %c ", ttt_fmt_square(pos, i, j));
        }

        if (i < pos->side_len - 1) {
            printf("\n___");

            for (int j = 1; j < pos->side_len; j++) {
                printf("|___");
            }
        }

        puts("");
    }

    for (int i = 0; i < pos->side_len - 1; i++) {
        printf("   |");
    }

    puts("\n");
}

int ttt_get_num(const char *failure_prompt) {
    for (;;) {
        int result;
        char buf[128];
        fgets(buf, sizeof buf, stdin);

        if (sscanf(buf, "%d", &result)) {
            return result;
        }
        
        printf("%s", failure_prompt);
    }
}

void ttt_get_move(struct TicTacToePosition *ttt_pos) {
    for (;;) {
        printf("Choose a square for %c's move: ", 
               ttt_current_player(ttt_pos));
        int move = ttt_get_num("Invalid input. Try again: ") - 1;
        int row = move / ttt_pos->side_len;
        int col = move % ttt_pos->side_len;

        if (ttt_try_move(ttt_pos, row, col)) {
            break;
        }

        puts("Invalid move. Pick an empty square between 1 and 9.");
    }
}

void ttt_play_game() {
    for (struct TicTacToePosition ttt_pos = ttt_init();;) {
        ttt_print_board(&ttt_pos);
        ttt_get_move(&ttt_pos);

        if (ttt_is_won(&ttt_pos)) {
            ttt_print_board(&ttt_pos);
            printf("%c won!\n", ttt_last_player(&ttt_pos));
            break;
        }
        else if (ttt_is_board_full(&ttt_pos)) {
            ttt_print_board(&ttt_pos);
            puts("The game is a draw");
            break;
        }
    }
}

int main() {
    ttt_play_game();
    return 0;
}

Answer 2

One-based indexing

Based on this:

int places[10] = {'o', '1', '2', ...

"\n\n"
,  places[1], places[2], ...

it seems that you're trying to push a square peg (one-based indexing) through a round hole (zero-based indexing). Try to use zero-based indexing instead.

Assuming ASCII

This:

places[i]-48

assumes that you're using ASCII for the compiler character literal encoding. That is often a correct assumption, but not necessarily a safe one. Since you have tested this on Linux it is likely that you are using gcc, so you should read about the f*charset options, and this question.

Aside from explicitly setting ASCII as the encoding, you should replace the above 48 with '0'.

Answer 3

As both players are handled the same:

// TODO: define marker = char array {' ', 'X', 'O'}

void switchPlayers(){
    printf("     Player %d choose: ", player);
    scanf("%d", &choice);
    // I inverted logic of the if!
    if (!markBoard(marker[player])) {
       player = 3 - player;
    }
    // ... the else can be dropped as player did not change and does not need to be changed
}

(Not a great improvement. In case of more players this would be wise.)

Answer 4

In addition to what's already been said, here's some misc remarks about using obsolete style:

• void switchPlayers(); Functions with an empty parenthesis is obsolete style in C since over 20 years back. You should be writing void switchPlayers(void); instead, because in C the () means "accept any parameter", which is type unsafe and outdated style.

  (Note that C and C++ are different here - in C++ () and (void) are equivalent.)

• Instead of using int with 1 or 0 for true/false, you should be using boolean types from stdbool.h: bool, true and false. The built-in C keyword _Bool type is also OK to use.

Answer 5

When I run the game, it allows me to select a side. I selected 'x'. Then the display changed to add the 'o' side, then the display commenced to flash with no (discernible) way for me/computer to make a move.

suggest the system( "clear" ); only be called after a side inputs a move.