HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Non-AI Tic-Tac-Toe program

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
nontoeticprogramtac

Problem

I'm fairly new to C++ and to game programming itself. Today I decided to build a multiplayer Tic Tac Toe program using only elemental C++ syntax/data structures. I also implemented a text interface for the game instead of real graphics.

Any suggestions or tips for the code?

```
#include
#include

#define GRID_SIZE 9

using namespace std;

void printGame(char gameGrid[]);
void inputToSlot(char gameGrid[], int slot, char choice);
void initialiseGame(char gameGrid[]);
bool isFull(char gameGrid[]);
bool isEmpty(char input);
bool checkWon(char gameGrid[]);
bool rowWon(char gameGrid[]);
bool columnWon(char gameGrid[]);
bool diagonalWon(char gameGrid[]);

int main(int argc, const char * argv[])
{
char gameGrid[GRID_SIZE];
initialiseGame(gameGrid);
int currentTurn = 0;

cout ";
} else {
cout ";
}
cin >> slotChoice;

while (!isEmpty(gameGrid[slotChoice])) {
cout ";
cin >> slotChoice;
}

// Insert
if (isP2) {
gameGrid[slotChoice] = 'o';
} else {
gameGrid[slotChoice] = 'x';
}

// Print
printGame(gameGrid);
cout << endl;

// Check Winner
if (checkWon(gameGrid)) {
if (isP2) {
cout << "Player 2 won the game.";
} else {
cout << "Player 1 won the game.";
}

break;
}

}

if (!checkWon(gameGrid)) {
cout << "Draw.";
}

}

void initialiseGame(char gameGrid[]) {
for (int i = 0; i < GRID_SIZE; ++i) {
gameGrid[i] = ' ';
}
}

void printGame(char gameGrid[]) {
cout << "+---+---+---+" << endl;
for (int i = 0; i < GRID_SIZE; ++i) {
cout << "| " << gameGrid[i] << " ";
if ((i+1) % 3 == 0 && i != 0) {
cout << "|" << endl;
cout << "+---+---+---+" << endl;
}
}
}

void inputToSlot(char gameGrid[], int slot, char choice) {

Solution

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  ";
    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

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

Code Snippets

cin >> slotChoice;

while (!isEmpty(gameGrid[slotChoice])) {
    cout << "Slot occupied. Please select another slot > ";
    cin >> slotChoice;
}
bool checkWon(const char gameGrid[]) {
   if (rowWon(gameGrid)) {
       return true;
   }
   if (columnWon(gameGrid)) {
       return true;
   }
   if (diagonalWon(gameGrid)) {
       return true;
   }

   return false;
}

Context

StackExchange Code Review Q#51987, answer score: 18

Revisions (0)

No revisions yet.