patterncppModerate
Non-AI Tic-Tac-Toe program
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) {
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';
}
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
Some style and refactoring suggestions, in no particular order...
-
I don't like the global
-
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
-
I think getting the
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
-
In the "Check Winner" section of the
-
I see some code duplication, especially in the code that generates output.
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.