6
\$\begingroup\$

I know several there have been several posts about Tic Tac Toe games previously, nonetheless I was really hoping that you may take a look at my attempt. I have tried to keep it as modularized as possible to minimize side effects. I started teaching myself C only about a month ago, so any and all advice would be greatly appreciated, especially in terms of general design and use of pointers.

Game

int main()
{
  int** board;
  int rounds_played, selection, col, row, current_player;

  current_player = O_PLAY;
  rounds_played = 0;

  board = initialize_board();
  print_instructions(board);

  while (rounds_played != MAX_ROUNDS)
  {
    /* Clear out current value in selection - prevents infinite loop if alphabetic char is entered */
    /* Side-effect requires you to hit enter prior to starting the game */
    while((selection = getchar()) != '\n' && selection != EOF);

    /* Get player selection */
    printf("Player %c, please make a move: ", current_player);
    scanf("%d", &selection);
    update_col_row(selection, &col, &row);

    /* Validate user-input and update board */
    if (is_valid_move(selection, col, row, board) == 1) {
      update_board(current_player, board, col, row);
      print_state(board);

      if (is_player_winner(board) > 0) {
        printf("Player %c wins the game!\n", current_player);
        return 0;
      }

      update_player(&current_player);
      rounds_played++;

    } else {
      printf("That move is not valid, please try again...\n");
      continue;
    }
  }

  free(board);
  printf("Game ended in a draw.\n");
  return 0;
}

Initialize Board

int** initialize_board()
{
  int** board;
  int i, j, tile_number;

  tile_number = 1;
  board = (int**) malloc(sizeof(int*) * BOARD_SIZE);

  for (i = 0; i < BOARD_SIZE; i++)
  {
    board[i] = malloc(sizeof(int) * BOARD_SIZE);
    for (j = 0; j < BOARD_SIZE; j++)
    {
      board[i][j] = tile_number;
      tile_number++;
    }
  }
  return board;
}

Place marker / update board

/* Place marker in selected tile */
void update_board(int marker, int** board, int col, int row)
{
  board[row][col] = marker;
}

Update player

 /* Toggle between players */
void update_player(int* current_player)
{
  *(current_player) = *(current_player) == X_PLAY ? O_PLAY : X_PLAY;
}

Validate Move

/* Validate user-input */
int is_valid_move(int selection, int col, int row, int** board)
{
  if (selection < 1 || selection > 9) {
    return 0;
  } else if (board[row][col] == X_PLAY || board[row][col] == O_PLAY) {
    return 0;
  }
  return 1;
}

Calculate Column / Row

/* Calculate horizontal and vertical axis based on user-input */
void calculate_col_row(int selection, int* col, int* row)
{
  selection -= 1;
  *(col) = selection % 3;
  *(row) = selection / 3;
}

Check Diagonal

/* Check major and minor diagonals for a winner */
int check_diagonal(int** board)
{
  if (board[0][0] == board[1][1] && board[1][1] == board[2][2]) {
    return 1;
  } else if (board[0][2] == board[1][1] && board[2][0] == board[1][1]) {
    return 1;
  }
  return 0;
}

Check Rows

/* Iterate through rows and check for a winner */
int check_rows(int** board)
{
  int i;

  for (i = 0; i < BOARD_SIZE; i++)
  {
    if (board[i][0] == board[i][1] && board[i][1] == board[i][2]) {
      return 1;
    }
  }
  return 0;
}

Check Columns

/* Iterate through columns and check for winner */
int check_columns(int** board)
{
  int i;
  for (i = 0; i < BOARD_SIZE; i++)
  {
    if (board[0][i] == board[1][i] && board[1][i] == board[2][i]) {
      return 1;
    }
  }
  return 0;
}

Check for winner

/* Check board for winner */
int is_player_winner(int** board)
{
  if (check_diagonal(board) || check_rows(board) || check_columns(board)) {
    return 1;
  }
  return 0;
}

Print current state

/* Print current state of the board */
void print_state(int **board)
{
  int i, j;
  printf("-------------\n");
  for (i = 0; i < BOARD_SIZE; i++)
  {
    for (j = 0; j < BOARD_SIZE; j++)
    {
      /* Hacky way to handle numbers and characters */
      if (board[i][j] > 9) printf("| %c ", board[i][j]);
      else printf("| %d ", board[i][j]);
    }
    printf("|\n-------------\n");
  }
}

Print instructions

/* Print game-play instructions */
void print_instructions(int **board)
{
  printf("\t\t\t\tWelcome to my Tic Tac Toe game\n\n");
  printf("\t\t\tWhen prompted to make a move, simply enter the number\n\t\t\t     corresponding to a tile of your choosing\n");
  printf("\t\t\t\tThe board will initalize as below: \n");
  printf("\t\t\t\t    Hit ENTER to start playing.\n");
  print_state(board);
}
\$\endgroup\$
3
\$\begingroup\$

Missing code

I had to guess at some definitions to make the code compile:

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

enum player {
    O_PLAY = 'O',
    X_PLAY = 'X',
};
#define BOARD_SIZE 3
#define MAX_ROUNDS (BOARD_SIZE * BOARD_SIZE)

Also, calculate_col_row was misspelt as update_col_row in main(). This suggests that you've not tested the code exactly as presented.

Memory leak

After a drawn game, we free() the board - that's good. We should do the same when a player wins, too. But what we forgot is that every element of board is a pointer to memory we allocated and have not yet freed. We probably want to encapsulate this in a function we can call from both places:

void free_board(int **board)
{
    for (int i = 0;  i < BOARD_SIZE;  ++i) {
        free(board[i]);
    }
    free(board);
}

When we use this, we get a clean Valgrind run.

Alternative representation

Instead of an array of arrays, consider a linear array of BOARD_SIZE * BOARD_SIZE elements. Then we don't need to convert to row/column representation apart from printing the board (checking for straight lines is also easier, as iterating rows, columns and diagonals then differ only in the 'step' from one position to the next). It also simplifies the allocation and deallocation, and it helps memory performance by keeping the values close together and not indirecting through a layer of pointers - these are issues that become more relevant when dealing with bigger matrices.

Consume excess input after reading

This comment, and the corresponding user message are a little bit of a niggle:

/* Side-effect requires you to hit enter prior to starting the game */

Instead, we could read up to the next newline after reading (and checking) the input value.

Treat predicates as boolean

These tests are non-idiomatic, and made me wonder if something strange is going on:

if (is_valid_move(selection, col, row, board) == 1) {
  if (is_player_winner(board) > 0) {

Functions beginning with is are expected to be predicates - i.e. pure functions returning zero (false) or non-zero (true). So let's use them as such:

if (is_valid_move(selection, col, row, board)) {
  if (is_player_winner(board)) {

Don't cast the result of malloc()

Once we include <stdlib.h>, we have prototypes for the allocation functions malloc(), calloc() and realloc(), so the compiler knows they return void*, and there's no need to explicitly cast to any other pointer type. It's also a good idea to use the dereferenced variable as the argument to sizeof, so that it remains consistent even if the type is changed:

int** initialize_board()
{
    int** board = malloc(sizeof *board * BOARD_SIZE);
    int tile_number = 1;
    for (int i = 0;  i < BOARD_SIZE;  i++) {
        board[i] = malloc(sizeof *board[i] * BOARD_SIZE);
        for (int j = 0;  j < BOARD_SIZE;  j++) {
            board[i][j] = tile_number++;
        }
    }
    return board;
}

A trick to swap between two values

We can alternate between two integers by using exclusive-or with a suitable value:

void update_player(int* current_player)
{
    *current_player ^= O_PLAY ^ X_PLAY;
}

However, it may be less clear, and the small efficiency gain is irrelevant here.

Use puts() for constant text, and combine strings

Instead of multiple printf() calls, we can puts() a single string to give instructions:

void print_instructions(int **board)
{
    puts("\t\t\t\tWelcome to my Tic Tac Toe game\n\n"
         "\t\t\tWhen prompted to make a move, simply enter the number\n"
         "\t\t\t     corresponding to a tile of your choosing\n"
         "\t\t\t\tThe board will initalize as below: \n"
         "\t\t\t\t    Hit ENTER to start playing.");
    print_state(board);
}

Note that puts() inserts a newline of its own at the end, so we don't supply one there.

\$\endgroup\$
  • \$\begingroup\$ Toby, thank you so much for taking the time to review my code. Your comments have been extremely helpful especially with regards to the memory leak, and also the thought of treating the board as a single linear area; that hadn't actually crossed my mind. I look forward to seeing you around in the future! \$\endgroup\$ – gustaf brostedt May 29 '18 at 20:13
2
\$\begingroup\$

Handle input properly

/* Clear out current value in selection - prevents infinite loop if alphabetic char is entered */
/* Side-effect requires you to hit enter prior to starting the game */
while((selection = getchar()) != '\n' && selection != EOF);
...
scanf("%d", &selection);

You should rather check the return value from scanf (e.g. if(scanf("%d", ...) != 1) handle_wrong_input() and/or maybe use getc+ungetc to ignore anything but digits:

int c;
do c = getc(stdin);
while (c != EOF and !isdigit(c));
if (c == EOF) return 0;
ungetc(c, stdin);
scanf...

Hacky ways vs. BOARD_SIZE

It appears that you have board of size 3x3 and never tested with bigger board, because this won't work for bigger board:

  /* Hacky way to handle numbers and characters */
  if (board[i][j] > 9) printf("| %c ", board[i][j]);
  else printf("| %d ", board[i][j]);

You know the number from coordinate (1+i*BOARD_SIZE+j), so, you only need three states (empty, O, X) and print the coordinate if it is empty. How about %02d if BOARD_SIZE > 3? You need some spaces or zeros to preserve width of columns.

Why #define/const if not using it?

/* Calculate horizontal and vertical axis based on user-input */
void calculate_col_row(int selection, int* col, int* row)
{
  selection -= 1;
  *(col) = selection % 3; //<--- BOARD_SIZE !!
  *(row) = selection / 3;
}

Why not e.g. 5-rows on 10x10 board?

/* Iterate through rows and check for a winner */
int check_rows(int** board)
...
    if (board[i][0] == board[i][1] && board[i][1] == board[i][2]) {

Style

No complains about style. Parts nicely separated (initialize_board, print_instructions, ...), consistent indentation. Good :)

...but provide full code with all the #define's (or enums/consts) next time, that I don't have to guess what e.g. X_PLAY is (looks like #define X_PLAY 'X').

Design and Pointers

I see no problem in the design or usage of pointers.

EDIT: removed complain about (int**) malloc(sizeof(int*) * BOARD_SIZE); My fault, it is correct.

\$\endgroup\$
2
\$\begingroup\$

There exists no reason why you need to use pointer-to-pointer here. It is slow and needlessly complex - a complexity that caused a memory leak bug.

Simply do int board [BOARD_SIZE][BOARD_SIZE];. Alternatively use a variable length array (VLA).

If you for reasons unknown must use dynamic memory allocation, then do so on a proper 2D array and not some fragmented look-up table. See Correctly allocating multi-dimensional arrays for examples of how to do this - it is likely that your book or teacher is teaching you bad habits here.

\$\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.