Xjyuk

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("
 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 continuen");
 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!

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.

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