Skip to main content
added 38 characters in body
Source Link
Jamal
  • 34.9k
  • 13
  • 133
  • 237

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

  • I don't like the global GRID_SIZEGRID_SIZE constant. I would make a structstruct (or a classclass) that holds gameGridgameGrid and the size. Then pass a reference to the structstruct (a constconst reference when possible) to the functions that currently have a gameGridgameGrid 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 currentTurncurrentTurn in the struct also.

  • I think getting the slotChoiceslotChoice 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;
     }
    
  • (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 boolbool named isP1 and rearrange several conditional statements.

  • In the "Check Winner" section of the whilewhile loop, you could save the return value from checkWon() in a local boolbool 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.

I made the parameter "const"const. I changed the cascading "else if"else if statements to three independent "if"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()RowWon(), columnWon()columnWon(), and diagonalWon()diagonalWon(). At the core of each is a complicated if statement that examines 3 cells and returns a bool result. But all of those ifif 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.

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.

  • 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;
     }
    
  • (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.

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.

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.

  • 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;
     }
    
  • (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.

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.

Source Link

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.