I wrote this C++ code for Tic-tac-toe for an interview (The basic skeleton was pre-provided) and got rejected because the code was too complex and inefficient. I'm not an expert programmer so I'd like some ideas about how to better my code to make it more efficient. The code is as follows:

#include <iostream>

const int N = 3; // Square NxN board, win with N consecutive

int board[N][N];

class TicTacToe {

 public:

int MakeMove(int player, int row_location, int col_location){

  if(player == 1){

    if(board[row_location][col_location]==0){

      board[row_location][col_location]=1;
    /*std::cout << "Player: "<< player << "\nCurrent board:\n";
    for(int i=0;i<N;i++){      
    for(int j=0;j<N;j++){
    std::cout << board[i][j];
    }
  }*/
}
else{

  std::cout<<"Invalid entry\n";
}
  }
  else if(player == 2){

if(board[row_location][col_location]==0){
  board[row_location][col_location]=2;
/*std::cout << "Player: "<< player << "\nCurrent board:\n";
for(int i=0;i<N;i++){      
    for(int j=0;j<N;j++){
        std::cout << board[i][j];
      }
  }*/
}
else{
  std::cout<<"Invalid entry\n";
}
  }

  int result = check_board(board);

  //std::cout << "\nResult of checkboard is: " << result << std::endl;

  return result;
}

private:

  int check_board(int board[N][N]){

bool flag = 0;
//std::cout << "\nInside checkboard function: \n";

if(flag==0)
{
for(int i=0;i<N;i++){      //Horizontal check
  for(int j=1;j<N-1;j++){
    if(board[i][j-1]==1 && board[i][j]==1 && board[i][j+1]==1){
      return 1;
      flag=1;
    break;
    }

    else if(board[i][j-1]==2 && board[i][j]==2 && board[i][j+1]==2){
      return 2;
      flag=1;
    break;
    }
  }
}
}

if(flag==0)
{

for(int j=0;j<N;j++){        //Vertical check
  for(int i=1;i<N-1;i++){

    if(board[i-1][j]==1 && board[i][j]==1 && board[i+1][j]==1){
      return 1;
      flag=1;
    break;
    }

    else if(board[i-1][j]==2 && board[i][j]==2 && board[i+1][j]==2){
      return 2;
      flag=1;
    break;
    }
  }
}
}

if(flag==0){

for(int i=1;i<N-1;i++){       //Leading diagonal check
  for(int j=i;j<N-1;j++){

    if(board[i-1][j-1]==1 && board[i][j]==1 && board[i+1][j+1]==1){
      return 1;
      flag=1;
    break;
    }

    else if(board[i-1][j-1]==2 && board[i][j]==2 && board[i+1][j+1]==2){
      return 2;
      flag=1;
      break;
    }
  }
}
}

if(flag==0){

 for(int i=N-1;i>1;i--){       //Opposite diagonal check

  for(int j=1;j<N-1;j++){

    //std::cout<<"In opposite diagonal loop\n "<<board[i+1][j-1]<<board[i][j]<<board[i-1][j+1];
    if(board[i+1][j-1]==1 && board[i][j]==1 && board[i-1][j+1]==1){

      return 1;
      flag=1;
    break;
    }

    else if(board[i+1][j-1]==2 && board[i][j]==2 && board[i-1][j+1]==2){

      return 2;
      flag=1;
      break;
    }
  }
} 
}
return 0;
}
};

int main(int argc, char** argv) {

  int result;

  TicTacToe ttt;

  std::cout << "Starting test!" << std::endl;

  result = ttt.MakeMove(1,1,0);

  result = ttt.MakeMove(1,1,1);

  result = ttt.MakeMove(1,1,2);

  std::cout << "Winning Player is: " << result << std::endl;

 return 0;

}
  • 2
    You really should at least make the effort of properly formatting your code. There are tools automating that, even. – Deduplicator Oct 11 at 2:29

I see a number of things that may help you improve your code.

Format your code

This code is messy and difficult to read. It has inconsistent indentation, and very little whitespace, making it hard to read and understand. There are abundant examples here of C++ code that is well formatted.

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea. In your code, there's no reason that board or N couldn't be members of the TicTacToe class instead, properly encapsulating.

Simplify your code

The code is full of duplicated code and inefficiency. Duplicated code is a sign that you either need to refactor (for example by creating a new function) or rewrite the logic in a more compact form. For example, there is no reason that horizontal and vertical checks couldn't be done within the same loop. Also, only the player that just made a move could possibly win, so there's no point in checking squares which aren't marked with the current player's token.

Use better names

The variable name board is OK, but the name flag is not. The first name explains something about what the variable means within the context of the code, but the latter is only confusing. Also check_board is vague and unhelpful. What is it checking? What does it return? Comments would help, but it's important to use descriptive names first.

Sanitize user input better

The only thing the code currently checks for is a space that's already occupied, but it does not verify that the proposed move is within bounds.

Eliminate "magic values"

The values of 1 and 2 are sprinkled through the code, but they really ought to be a named constant instead, and specifically a named constant static member of the class.

Fix the bug

The "opposite diagonal check" has these lines (formatting fixed):

for (int i = N - 1; i > 1; i--) {   //Opposite diagonal check
    for (int j = 1; j < N - 1; j++) {
        if (board[i + 1][j - 1] == 1 && board[i][j] == 1

The first time through the loop i - N - 1, but the check is looking at board[i + 1][j - 1] which will be board[N][0] the first time through the loop. That's out of the boundary of the board and thus, a bug.

Think of the user

What happens if there is a tie game? The board is full, but neither player has won. The code doesn't check for or recognize this state, but it should, to avoid a frustrating experience for a user.

Study other code

One way to become a better programmer is to ask for a review of your code. You've just done that, so good job! Another way that helps is to read other people's code. Look at other implementations of this game on Code Review and carefully read the answers. It's a very simple way to learn a lot of good information.

  • Thank you so much for the very comprehensive review, I started seeing all these problems only as I was reading through your comments. That is very helpful. – Prasanth Suresh Oct 12 at 16:08

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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