patterncppCriticalCanonical
Tic Tac Toe C++
Viewed 0 times
tactictoe
Problem
I made this Tic Tac Toe game and I would like to know your opinion and what can be improved. I also want advice about making it check if someone wins. I'm 10th grader, so I still need to learn a lot.
#include
#include
using namespace std;
int main()
{
char x1,x2,x3,x4,x5,x6,x7,x8,x9;
int x, i, a;
x1=x2=x3=x4=x5=x6=x7=x8=x9='+';
i=0;
cout > x;
if(x==1 && x1!='X' && x1!='O')if(a%2==0)x1='X', a++, i++;
else x1='O', a++, i++;
if(x==2 && x2!='X' && x2!='O')if(a%2==0)x2='X', a++, i++;
else x2='O', a++, i++;
if(x==3 && x3!='X' && x3!='O')if(a%2==0)x3='X', a++, i++;
else x3='O', a++, i++;
if(x==4 && x4!='X' && x4!='O')if(a%2==0)x4='X', a++, i++;
else x4='O', a++, i++;
if(x==5 && x5!='X' && x5!='O')if(a%2==0)x5='X', a++, i++;
else x5='O', a++, i++;
if(x==6 && x6!='X' && x6!='O')if(a%2==0)x6='X', a++, i++;
else x6='O', a++, i++;
if(x==7 && x7!='X' && x7!='O')if(a%2==0)x7='X', a++, i++;
else x7='O', a++, i++;
if(x==8 && x8!='X' && x8!='O')if(a%2==0)x8='X', a++, i++;
else x8='O', a++, i++;
if(x==9 && x9!='X' && x9!='O')if(a%2==0)x9='X', a++, i++;
else x9='O', a++, i++;
system("CLS");
}
cout<< x7 << "I" << x8 << "I" << x9 << endl
<< x4 << "I" << x5 << "I" << x6 << endl
<< x1 << "I" << x2 << "I" << x3 << endl;
cout<< "Who won?\n";
system("pause");
return 0;
}Solution
Avoid platform specific code
If there is an alternative you should try to avoid platform specific code.
This entails avoiding
There are guides on how to wait for user input on the command line and on how to clear the screen on multiple platforms on SO.
I had to adapt your code to make it run on my machine.
Use arrays for repetitive data
You should definitely use an array instead of single variables.
would become
or even two dimensional
You would have to change to zero based indexing for this so
Use loops for repetitive tasks
The array opens up loop usage. For example, you might want to do the printing in a loop instead of a row. For the 2D array this would be:
There are multiple occasions where you print the board in some way or another.
This calls for a function that does the printing:
This function can be used like:
or like
Don't abuse the comma operator
In your code you have many lines like
You should not misuse the comma operator to chain multiple commands into an if. Instead, you should separate the commands by
Factor out common code
There are many similarities in the different branches of the
Generally, you should always use
Use the ternary operator where it improves readability
The nested
becomes
Separate validity checks from actions
Finally, you have the list of
First, we need to adapt that to the 2D array. To do so, we have to calculate the row and column from the position
Duplicate variables
Reducing the code showed that variables
I also noticed that
Check user input, return meaningful errors
Every time we read values from the user, we have to consider what happens when the user enters an invalid value.
While your code does nothing wrong thanks to the many
and similarly:
Naming
Names are important for the understanding of variables' meanings.
Generally, the farther away from the declaration a name is used
If there is an alternative you should try to avoid platform specific code.
This entails avoiding
#include and using things like system("pause"); which rely on commands the OS understands.There are guides on how to wait for user input on the command line and on how to clear the screen on multiple platforms on SO.
I had to adapt your code to make it run on my machine.
Use arrays for repetitive data
You should definitely use an array instead of single variables.
char x1,x2,x3,x4,x5,x6,x7,x8,x9;would become
char fields[9];or even two dimensional
char fields[3][3];You would have to change to zero based indexing for this so
x4 would become fields[3] or fields[1][0] in the two dimensional case.Use loops for repetitive tasks
The array opens up loop usage. For example, you might want to do the printing in a loop instead of a row. For the 2D array this would be:
for(int row = numberOfRows - 1; row >= 0; --row) {
for(int col = 0; col < numberOfColumns; ++col) {
cout << fields[row][col];
if(col < 2) {
cout << "I";
} else {
cout << "\n";
}
}
}There are multiple occasions where you print the board in some way or another.
This calls for a function that does the printing:
void print(Board fields, std::string columnSeparator = "I", std::string rowSeparator = "\n") {
for(int row = 2; row >= 0; --row) {
for(int col = 0; col < 3; ++col) {
cout << fields[row][col];
if(col < 2) {
cout << columnSeparator;
} else {
cout << rowSeparator;
}
}
}
}This function can be used like:
print(fields);
cout << "Tic Tac Toe\n";or like
Board numbers{{'1', '2', '3'},
{'4', '5', '6'},
{'7', '8', '9'}};
print(numbers);Don't abuse the comma operator
In your code you have many lines like
if(x==1 && x1!='X' && x1!='O')if(a%2==0)x1='X', a++, i++;
else x1='O', a++, i++;You should not misuse the comma operator to chain multiple commands into an if. Instead, you should separate the commands by
; as usual and group them into a common scope like so:if(x==1 && x1!='X' && x1!='O') {
if(a%2==0) {
x1='X';
a++;
i++;
} else {
x1='O';
a++;
i++;
}
}Factor out common code
There are many similarities in the different branches of the
if in the code above. When the trailing code of two branches of the same if is the same, you can extract it out of the if:if(x==1 && x1!='X' && x1!='O') {
if(a%2==0) {
x1='X';
} else {
x1='O';
}
a++;
i++;
}Generally, you should always use
{} after control flow constructs like if, while, or for to avoid errors where adding a line does not add it to the correct loop/if.Use the ternary operator where it improves readability
The nested
if in the above code can be replaced by the ternary operator:if(a%2==0) {
x1='X';
} else {
x1='O';
}becomes
x1= (a%2==0) ? 'X' : 'O';Separate validity checks from actions
Finally, you have the list of
ifs that should set the appropriate field.First, we need to adapt that to the 2D array. To do so, we have to calculate the row and column from the position
x:const int row = (x - 1) / numberOfColumns;
const int column = (x - 1) % numberOfColumns;
if(fields[row][column] == '+') {
fields[row][column] = (a%2==0) ? 'X' : 'O';
a++;
i++;
}Duplicate variables
Reducing the code showed that variables
i and a do exactly the same and will therefore have the same value. So why have two of them?I also noticed that
a was not initialized which results in undefined behavior.Check user input, return meaningful errors
Every time we read values from the user, we have to consider what happens when the user enters an invalid value.
While your code does nothing wrong thanks to the many
ifs, it would be better to make the input checks more explicit in the code and to tell the user what has gone wrong:do{
cout > x;
if(x 9) {
cout << "Invalid value: Expected integer between 1 and 9!\n";
}
} while(!(0 < x && x < 10));and similarly:
if(fields[row][column] == '+') {
fields[row][column] = (i%2==0) ? 'X' : 'O';
i++;
} else {
cout << "Field already taken!\n";
}Naming
Names are important for the understanding of variables' meanings.
Generally, the farther away from the declaration a name is used
Code Snippets
char x1,x2,x3,x4,x5,x6,x7,x8,x9;char fields[9];char fields[3][3];for(int row = numberOfRows - 1; row >= 0; --row) {
for(int col = 0; col < numberOfColumns; ++col) {
cout << fields[row][col];
if(col < 2) {
cout << "I";
} else {
cout << "\n";
}
}
}void print(Board fields, std::string columnSeparator = "I", std::string rowSeparator = "\n") {
for(int row = 2; row >= 0; --row) {
for(int col = 0; col < 3; ++col) {
cout << fields[row][col];
if(col < 2) {
cout << columnSeparator;
} else {
cout << rowSeparator;
}
}
}
}Context
StackExchange Code Review Q#123200, answer score: 57
Revisions (0)
No revisions yet.