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

Tic Tac Toe C++

Submitted by: @import:stackexchange-codereview··
0
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 #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.