5
\$\begingroup\$

I'm learning myself C and to make sure that I learn to code C in a proper way I created tic tac toe and want to hear your opinion on what I should do and what I shouldn't do.

One thing is sure is to divide the code up in several files maybe but outside of that.

The code:

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

#define ROWS 3
#define BOARD_SIZE ROWS * ROWS

typedef enum { false, true } bool;

bool enter_entry(char *board, char character, int x, int y);
void print_board(const char *board);
void *init_board();
bool is_solved(char *board, char item);
bool is_draw(char *board);
bool is_equal(char *board, int indexA, int indexB, int indexC, char item);
void player_selection(char *board, char item);
void game_loop(char *board);

int main()
{
    char *board = init_board();
    game_loop(board);
    return 0;
}

void *init_board()
{
    char *board = malloc(sizeof(char) * BOARD_SIZE);
    for(int i = 0; i < BOARD_SIZE; i++)
    {
        board[i] = ' ';
    }
    return board;
}

void print_board(const char *board)
{
    for(int i = 1; i < BOARD_SIZE+1; i++)
    {
        printf("| %c ", board[i-1]);
        if(i % 3 == 0)
        {
            printf("|\n");        
        }
    }
    printf("\n");
}

bool is_solved(char *board, char item)
{
    if(is_equal(board, 0, 1, 2, item)) { return true; }
    if(is_equal(board, 3, 4, 5, item)) { return true; }
    if(is_equal(board, 6, 7, 8, item)) { return true; }

    if(is_equal(board, 0, 3, 6, item)) { return true; }
    if(is_equal(board, 1, 4, 7, item)) { return true; }
    if(is_equal(board, 2, 5, 8, item)) { return true; }

    if(is_equal(board, 0, 4, 8, item)) { return true; }
    if(is_equal(board, 2, 4, 6, item)) { return true; }
    return false;
}

bool is_draw(char *board)
{
    for(int i = 0; i < BOARD_SIZE; i++)
    {
        if(board[i] == ' ')
        {
            return false;
        }
    }
    return true;
}

bool is_equal(char *board, int indexA, int indexB, int indexC, char item)
{
    if(board[indexA] == item && board[indexB] == item && board[indexC] == item)
    {
        return true;
    }
    return false;
}

bool enter_entry(char *board, char character, int x, int y)
{
    int index = x + ROWS * y;

    if(board[index] != ' ')
    {
        return false;
    }

    board[index] = character;
    return true;
}

void player_selection(char *board, char item)
{
    int x, y;
    printf("enter coords (x, y): \n");

    while(1)
    {
        scanf(" %d %d", &x, &y);
        bool succes = enter_entry(board, item, x-1, y-1);
        if(succes)
        {
            break;
        }
        printf("This coord is already used or not valid enter new ones:\n");
    }
    print_board(board);
}

void game_loop(char *board)
{
    char playerOneChar = 'o';
    char playerTwoChar = 'x';

    printf("Welcome to tic tac toe!\n");
    printf("Press enter to continue\n");
    char enter = 0;
    while (enter != '\r' && enter != '\n') { enter = getchar(); }

    printf("Let's start the game!\n");
    print_board(board);

    while(1)
    {
        printf("Player one: \n");
        player_selection(board, playerOneChar);
        if(is_solved(board, playerOneChar))
        {
            printf("Player one won!\n");
            break;
        }

        if(is_draw(board))
        {
            printf("No winners!\n");
            break;
        }

        printf("Player two: \n");
        player_selection(board, playerTwoChar);
        if(is_solved(board, playerTwoChar))
        {
            printf("Player two won!");
            break;
        }
        if(is_draw(board))
        {
            printf("No winners!\n");
            break;
        }
    }
}

Thank you in advance!

\$\endgroup\$
5
\$\begingroup\$

malloc

I start with one of the most common ones. Instead of char *board = malloc(sizeof(char) * BOARD_SIZE) write char *board = malloc(sizeof(*board) * BOARD_SIZE). If you decide to change the type in the future, you don't have to change at more than one place. And besides, sizeof(char) is ALWAYS 1.

But the biggest problem is that you're not checking the return value. It should look like this:

void *init_board()
{
    char *board = malloc(sizeof(char) * BOARD_SIZE);
    if(!board) { 
        /* Handle error */ 
    } else {     
        for(int i = 0; i < BOARD_SIZE; i++) {
            board[i] = ' ';
        }
    }
    return board;
}

Or like this:

void *init_board()
{
    char *board = malloc(sizeof(char) * BOARD_SIZE);
    if(board) {
        for(int i = 0; i < BOARD_SIZE; i++) {
            board[i] = ' ';
        }
    }
    return board;
}

But if you choose the latter one, then you need to check the return value of init_board().

Another thing about the board variable. Why not make it into a 3x3 array instead? It's overkill to call malloc for a 9 byte array. I would do like this instead:

const int dim=3;

void init_board(char board[dim][dim]) 
{
    for(int i=0; i<dim; i++)
        for(int j=0; j<dim, j++)
            board[i][j]=' ';
}

And then in main()

char board[3][3];
init_board(board);

scanf

You're also not checking the return value of scanf. That should also always be done.

scanf(" %d %d", &x, &y);

should be

if(scanf(" %d %d", &x, &y) != 2) {
    /* Handle error */
} else {

But if you ask me, the best method, even though it takes a few more lines, is this:

const size_t buffer_size = 100;
char buffer[buffer_size];
if(!fgets(buffer, buffer_size, stdin)) {
    /* Handle error */
} else {
    if(sscanf(buffer, "%d %d", &x, &y) != 2) {
        /* Handle error */
    }
}

bool

No reason to define bool, true and false on your own. Just include stdbool.h.

const

A minor thing is that you should declare playerOneChar and playerTwoChar as const.

style

This is my personal preference, but I think you waste a lot of space with unnecessary braces. I would at least move the opening brace to then end of previous line, except for functions. Like this:

bool is_draw(char *board)
{
    for(int i = 0; i < BOARD_SIZE; i++) {
        if(board[i] == ' ') {
            return false;
        }
    }
    return true;
}

or even

bool is_draw(char *board)
{
    for(int i = 0; i < BOARD_SIZE; i++) 
        if(board[i] == ' ') 
            return false;
    return true;
}

Remember that readability also includes not having to scroll more than necessary. Putting the opening brace on the line before or sometimes even removing them completely barely makes it harder to read at all. If you ask me it's even easier. But it can save you a ton of lines, making more code visible at the same time. In the above example I would probably remove the braces for the if statement, but keep the braces for the for loop.

I usually go by the coding style guide for the Linux kernel, except that I prefer a tab size of 4 instead of 8.

Apart from these things, I think it looks pretty good. Nice work.

\$\endgroup\$
  • \$\begingroup\$ Thanks Broman for your comments, I will take them into account! Thanks! \$\endgroup\$ – John Jun 8 at 18:30
  • \$\begingroup\$ Does this code really work? See stackoverflow.com/questions/2828648/… \$\endgroup\$ – Roland Illig Jun 9 at 13:02
  • \$\begingroup\$ I do not agree with the modification of the brace style. It is much better to align the braces vertically rather than scattering them left and right, all over the page \$\endgroup\$ – user3629249 Jun 9 at 13:55
  • \$\begingroup\$ the number of lines used is not important. Extra lines for the braces makes the code much easier for humans to read and the compiler does not care \$\endgroup\$ – user3629249 Jun 9 at 13:57
  • \$\begingroup\$ @user3629249 I do not agree that those extra lines makes it easier. Empty lines CAN make it easier to read, but they can also do the opposite if used the wrong way. \$\endgroup\$ – klutt Jun 9 at 16:02

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.