2
\$\begingroup\$

I have tried to write a simple 3 x 3 tic tac toe program. Please I need a code review on this program, whether it is readable, maintainable or not. Judge it as much as you can.

Here is the source code:

#include <iostream>
#include <string>
#include <vector>


void draw_board(std::vector<std::string> &board) {

  /**
  -----------------------
 |       |       |       |
 |   1   |   2   |   3   |
 |       |       |       |
 | --------------------  |
 |       |       |       |
 |   4   |   5   |   6   |
 |       |       |       |
 | --------------------  |
 |       |       |       |
 |   7   |   8   |   9   |
 |       |       |       |
  -----------------------
  **/
  for (auto row : board) {
    for (auto col : row) {
      std::cout << col;
    }
    std::cout << '\n';
  }

}

void toggle_player(char &current) {
  if (current == 'X') current = 'O';
  else current = 'X';
}

void update_board(char &current, std::vector<std::string> &board, int &choice,
                  std::vector<std::pair<int, int>> &choice_position, std::vector<std::string> &plays) {
  int row = choice_position[choice].first;
  int col = choice_position[choice].second;
  board[row][col] = current;
  switch (choice) {
  case 1:
    plays[0][0] = current;
    break;
  case 2:
    plays[0][1] = current;
    break;
  case 3:
    plays[0][2] = current;
    break;
  case 4:
    plays[1][0] = current;
    break;
  case 5:
    plays[1][1] = current;
    break;
  case 6:
    plays[1][2] = current;
    break;
  case 7:
    plays[2][0] = current;
    break;
  case 8:
    plays[2][1] = current;
    break;
  case 9:
    plays[2][2] = current;
    break;
  }

}

bool valid_move(int &choice, std::vector<bool> &taken) {
  return 1 <= choice && choice <= 9 && !taken[choice];
}

void handle_input(char &current, std::vector<bool> &taken, std::vector<std::string> &plays
                  ,std::vector<std::pair<int, int>> &choice_position
                  ,std::vector<std::string> &board) {
  std::cout << "Enter a number between [1, 9] (inclusive).\n";
  std::cout << "It's " << current << " turn\n";
  int choice;
  std::cin >> choice;
  while(!valid_move(choice, taken)) {
    std::cout << "Invalid choice\n";
    std::cin >> choice;
  }
  taken[choice] = true;
  update_board(current, board, choice, choice_position, plays);
  toggle_player(current);
}



std::string winner(std::vector<std::string> &plays) {
  // check rows
  std::string winner;
  for (int i = 0; i < 3; i++) {
    int cnt = 0;
    for (int j = 0; j < 3; j++) {
      if (plays[i][j] == plays[i][0])
          cnt++;
    }
    if (cnt == 3 && plays[i][0] != '.') {
      winner += plays[i][0];
      winner += " wins the game.";
      return winner;
    }
  }
  // check columns
  for (int i = 0; i < 3; i++) {
    int cnt = 0;
    for (int j = 0; j < 3; j++) {
      if (plays[j][i] == plays[0][i])
          cnt++;
    }
    if (cnt == 3 && plays[0][i] != '.') {
      winner += plays[0][i];
      winner += " wins the game.";
      return winner;
    }
  }
  // check diagonals
  if (plays[0][0] == plays[1][1] && plays[1][1] == plays[2][2] && plays[1][1] != '.') {
    winner += plays[1][1];
    winner += " wins the game";
    return winner;
  }
  if (plays[0][2] == plays[1][1] && plays[1][1] == plays[2][0] && plays[1][1] != '.') {
    winner += plays[1][1];
    winner += " wins the game";
    return winner;
  }
  return "-1"; // no winner
}
void handle_winner(std::vector<std::string> &plays, std::vector<std::string> &board,
                   std::vector<std::pair<int, int>> &choice_position);

void start_game(std::vector<std::string> &board, std::vector<std::pair<int, int>> &choice_position) {
  // initial state of the game
  char current = 'X';
  std::vector<std::string> plays(3);
  plays[0] = plays[1] = plays[2] = "...";
  std::vector<bool> taken(10);
  board = {" ------------------------", "|       |       |        |", "|   1   |   2   |   3    |",
  "|       |       |        |", " ------------------------", "|       |       |        |", "|   4   |   5   |   6    |",
  "|       |       |        |", " ------------------------", "|       |       |        |", "|   7   |   8   |   9    |",
  "|       |       |        |", " ------------------------"
  };
  while (true) {
    draw_board(board);
    handle_input(current, taken, plays, choice_position, board);
    handle_winner(plays, board, choice_position);

  }
}
void handle_winner(std::vector<std::string> &plays, std::vector<std::string> &board,
                   std::vector<std::pair<int, int>> &choice_position) {
  if (winner(plays) != "-1") {
    std::cout << winner(plays);
    std::cout << "Do you want to play another game?\n";
    std::cout << "Enter 0 to exit, 1 to continue\n";
    int choice;
    std::cin >> choice;
    if (choice == 0)
      exit(0);
    start_game(board, choice_position);
  }
}



int main() {
  std::vector<std::string> board{" ------------------------", "|       |       |        |", "|   1   |   2   |   3    |",
  "|       |       |        |", " ------------------------", "|       |       |        |", "|   4   |   5   |   6    |",
  "|       |       |        |", " ------------------------", "|       |       |        |", "|   7   |   8   |   9    |",
  "|       |       |        |", " ------------------------"
  };
  std::vector<std::pair<int, int>> choice_position(10);
  // locations of numbers on the board
  choice_position[1] = {2, 4}; 
  choice_position[2] =  {2, 12};
  choice_position[3] = {2, 20};
  choice_position[4] = {6, 4};
  choice_position[5] = {6, 12};
  choice_position[6] = {6, 20};
  choice_position[7] = {10, 4};
  choice_position[8] = {10, 12};
  choice_position[9] = {10, 20};
  start_game(board, choice_position);
  return 0;
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

It has been a long time since I did something in c++ so I probably cannot give any pointers on c++ specific things.

The first thing I notice is the data types you use to encode data. For example, you encode the board as a vector of strings std::vector<std::string> &board which has the entire printable board. Instead of encoding the board in this way you should instead ask what data represents a board. In the case of tic tac toe it is a grid of 3 by 3 with each square encoding information if it is empty, filled by X or filled by O. Printing the board can then be offloaded to the draw_board method. In this way the other methods using the board don't need to know how it is printed/drawn. You can create a struct out of the board (or even a class if you want to go that route)

struct Board {
  int pieces[3][3];
}

if you decide to encode the pieces on the board as ints (more on that later). You can even add more information to the Board struct, for example the current player to move, how many moves the game has advanced etc.

The same holds for the player encoding. You now use the chars X and O but you can also create an enum to state a player, for example

enum Player { Xs, Os }

This prevents you from making errors in the future where for example you typed 'P' instead of 'O'. Using enums will result in compiler errors if the enum value does not exist.

You can also encode the squares on the board with an enum. Each square has only three possibilities: filled by Xs, filled by Os or empty. So a square can be

enum Square { Empty, X, O }

or something like that (maybe not X and O as that might give collisions in the future but you get the point).

The goal for these enums and structs is to abstract the data you are working with (instead of working with a vector of strings, work with a Board) and to give a more clear expression of what the code is doing (instead of providing a char, provide a Player). This makes code more readable and also prevents some mental gymnastics in the future.

In your update_board there is a switch case with a whopping 9 cases. Usually when I see a switch case with a lot of cases I try to reduce the amount of cases or even try to get rid of the switch case because all the cases are a lot of copy-pasting and can have possible errors in them. In this case, your choice is a value between 1 and 9. This is fine for the player input, but it might be better to use the values 0 to 8 for this as soon as you got the input from the player. In that way figuring out which item in plays to edit can be easily calculated using the division / and modules operators %:

plays[choice / 3][choice % 3] = current

Of course, it is a very good idea to replace the 3s in the above example with some variables like boardHeight and boardWidth so it is clear what the 3 means and if you would like to create a different version with a 4 by 4 board for example, you wouldn't need to change all the 3s in your entire source code with 4s.

About the winner function. It might be better to have a method which expects a board and player as input and returns a boolean wether this player won as you only need to check for the player who just moved if if he won. This also makes you checking the different squares easier as you don't need to check the squares with each other but just check if each square matches the players piece/symbol. You can choose to write each case (ie each column, each row and each diagonal) instead of using for loops, in this case there are only 8 cases to check and it makes it a bit more clear, but the major downside is that you cannot change it that easily if you want to modify the game in the future to some other version as I mentioned earlier.

I'll leave it at this, my main takeaway is to use data types and abstractions for the different pieces of information/data you use. You'll see that if you use these abstractions you'll realise that you can write some pieces of code a lot easier. I won't comment you on c++ specific styles, someone else can do that for you ;)

\$\endgroup\$
3
  • \$\begingroup\$ Raw arrays are generally discouraged in C++. Multidimensional raw arrays are even more frowned upon ... Also, enum class is preferable to enum (the former doesn't have collision problems). But otherwise, good review! \$\endgroup\$
    – L. F.
    Mar 25, 2020 at 6:55
  • \$\begingroup\$ Thanks. I guess one should use std::array instead of raw arrays? \$\endgroup\$
    – pepijno
    Mar 25, 2020 at 7:59
  • \$\begingroup\$ Yeah, std::array<Square, 9> or std::array<std::array<Square, 3>, 3> would be fine. \$\endgroup\$
    – L. F.
    Mar 25, 2020 at 8:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.