Return to Revisions

Overall, I think it is a pretty good program.

I think I see one bug. You don't check that the user's input for slotChoice is valid. If the input is outside the range 0..8, the program overruns the array.

Some style and refactoring suggestions, in no particular order...

• I don't like the global GRID_SIZE constant. I would make a struct (or a class) that holds gameGrid and the size. Then pass a reference to the struct (a const reference when possible) to the functions that currently have a gameGrid parameter.

• Later, if you want to make the program more object-oriented, you can make most of the current functions into member functions of the class. Since you are new to C++, I would treat that as a separate project.

• Consider putting currentTurn in the struct also.

• I think getting the slotChoice input and validating it is a good bit of work to move to a new function. I would move this:

   cin >> slotChoice;
  
   while (!isEmpty(gameGrid[slotChoice])) {
       cout << "Slot occupied. Please select another slot > ";
       cin >> slotChoice;
   }
  

plus new code to check that the input is in range, to a new function.

• (Small point) I like to read things in their natural order. I'd rather see the "stuff" for player 1 come before the "stuff" for player 2. So I'd use a bool named isP1 and rearrange several conditional statements.

• In the "Check Winner" section of the while loop, you could save the return value from checkWon() in a local bool variable. Then you wouldn't need to call the function again to decide if the game was a draw.

• I see some code duplication, especially in the code that generates output.

cout << "Player 2 won the game."; appears again, with only 1 character changed.

`cout << "+---+---+---+" << endl;` appears twice, and could go in a separate function.

• An alternative for checkWon():

   bool checkWon(const char gameGrid[]) {
      if (rowWon(gameGrid)) {
          return true;
      }
      if (columnWon(gameGrid)) {
          return true;
      }
      if (diagonalWon(gameGrid)) {
          return true;
      }
  
      return false;
   }
  

I made the parameter const. I changed the cascading else if statements to three independent if statements. I think that reflects the underlying logic a bit better; the three win conditions aren't mutually exclusive.

• You could simplify the logic in function rowWon(). If you check isEmpty(firstInRow), you don't need to also check isEmpty(secondInRow) and isEmpty(thirdInRow). (The later comparisons with firstInRow will exclude cases where the other cells in the row are empty.) columnWon() and diagonalWon() could get the same simplification.

• There's a lot of code duplication (and conceptual duplication) between RowWon(), columnWon(), and diagonalWon(). At the core of each is a complicated if statement that examines 3 cells and returns a bool result. But all of those if statements are essentially the same. That decision could go in a a separate function -- pass it the 3 cell values. The other part of each function involves deciding which 3 cells to test; those decisions are different for each win condition.