Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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<iostream>
#include<windows.h>
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 << "+I+I+\n" << "+I+I+\n" << "+I+I+\n";
cout << "Tic Tac Toe\n";
system("pause");
system("CLS");

cout << "7I8I9\n" << "4I5I6\n" << "1I2I3\n";
cout << "Board with coordinates\n";
system("pause");
system("CLS");

while(i<9)
{
        cout<< x7 << "I" << x8 << "I" << x9 << endl
            << x4 << "I" << x5 << "I" << x6 << endl
            << x1 << "I" << x2 << "I" << x3 << endl;
        if(a%2==0)cout << "X Turn\n";
        else      cout << "O Turn\n";
        cout << "Type coordinate of square(number) "; cin >> 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;
}
share|improve this question
2  
Welcome to CR. As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. – Nobody Mar 18 at 11:26

Avoid platform specific code

If there is an alternative you should try to avoid platform specific code. This entails avoiding #include <windows.h> 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 << "Type coordinate of square(number) ";
            cin >> x;
            if(x < 1 || 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 the more descriptive it needs to be. In your code I would rename as follows:

  • x1, x2, ... -> fields
  • x -> fieldIndex
  • i -> roundIndex
  • a -> removed as it duplicates i

Misc

You don't need to return 0; from main in C++. This is done by default if main ends before a return is encountered.

Don't using namespace std; it will bring you more trouble than it saves in typing.

Use correct formatting to improve the readability of your code. Usually, each level of nesting ({}) deepens the indentation.

Code

This is the code that I came up with. There are still some issues that I would correct but this post has become long enough as it is. (Note that I have removed the screen clearing but it should be possible to reinsert it in a multi platform manner.)

#include <iostream>

constexpr int numberOfRows = 3;
constexpr int numberOfColumns = 3;

using Board = char[numberOfRows][numberOfColumns];

void print(Board fields, std::string columnSeparator = "I",
           std::string rowSeparator = "\n") {
  for (int row = numberOfRows - 1; row >= 0; --row) {
    for (int col = 0; col < numberOfColumns; ++col) {
      std::cout << fields[row][col];
      if (col < numberOfColumns - 1) {
        std::cout << columnSeparator;
      } else {
        std::cout << rowSeparator;
      }
    }
  }
}

int main() {
  Board fields;
  for (int row = 0; row < numberOfRows; ++row) {
    for (int column = 0; column < numberOfColumns; ++column) {
      fields[row][column] = '+';
    }
  }
  int fieldIndex, roundIndex = 0;

  print(fields);
  std::cout << "Tic Tac Toe\n";
  std::cin.get();

  Board numbers{{'1', '2', '3'}, {'4', '5', '6'}, {'7', '8', '9'}};
  print(numbers);
  std::cout << "Board with coordinates\n";
  std::cin.get();

  while (roundIndex < 9) {
    print(fields);
    bool xsTurn = roundIndex % 2 == 0;
    if (xsTurn) {
      std::cout << "X Turn\n";
    } else {
      std::cout << "O Turn\n";
    }

    do {
      std::cout << "Type coordinate of square(number) ";
      std::cin >> fieldIndex;
      if (fieldIndex < 1 || fieldIndex > 9) {
        std::cout << "Invalid value: Expected integer between 1 and 9!\n";
      }
    } while (!(0 < fieldIndex && fieldIndex < 10));

    const int row = (fieldIndex - 1) / numberOfColumns;
    const int column = (fieldIndex - 1) % numberOfColumns;

    if (fields[row][column] == '+') {
      fields[row][column] = (xsTurn) ? 'X' : 'O';
      roundIndex++;
    } else {
      std::cout << "Field already taken!\n";
    }
  }
  print(fields);
  std::cout << "Who won?\n";
  std::cin.get();
} 
share|improve this answer
10  
+1 Good review. If you also mention a caution about using namespace std, advocate for a little whitespace and suggest the use of an object to represent the game, you'll have covered most everything I would have written. – Edward Mar 18 at 12:59
1  
Great answer, just wanted to point out that char fields[9] = { '+' } is equivalent to x1='+';x2=x3=x4=x5=x6=x7=x8=x9='\0'; and not to the original code x1=x2=x3=x4=x5=x6=x7=x8=x9='+';: the array is assigned according to the initializer with default initialization for any remaining elements. – CompuChip Mar 18 at 13:48
1  
@CompuChip: You're right. I removed that in the final code but forget it in this section. Also note that a similar problem occurs in the 2D array initialization. – Nobody Mar 18 at 13:50
1  
Rather than print(), I'd be inclined to do things the C++ way, and override the << operator, ostream& operator<<(ostream& os, const Board& board) {...}. Then you can just count << board`, which is much more C++esque. Other than that, heartily agree with this answer. – Dewi Morgan Mar 18 at 15:17
1  
@MatthieuM.: Pure oversight. There was so much to review and even now I see this as a little detail in comparison to the other "problems". – Nobody Mar 18 at 15:20

I am adding to what @Nobody said. It is a good idea to manage the game state in a class, since you have tagged the question C++. Although there are more lines of code in this version, it helps to organize code greatly

main.cpp

#include "game.h"

using std::cout;
using std::cin;
using std::cerr;

void getrc(size_t *row, size_t *col)
{
    for (;;) {
        cout << "Enter the row and column position\n";
        if (cin >> *row >> *col)
            break;
        else {
            cin.clear();
            cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
    }
}

int main()
{
    game g;
    size_t row, col;
    winstate win;
    try {
        cout << "You are " << square2char(g.gethuman()) << '\n';
        do {
            getrc(&row, &col);
            g.set(row - 1, col - 1);
            g.pcset();
            cout << g;
            getchar();
        } while (((win = g.getwinner()) == winstate::NOWIN) && 
                  (win != winstate::TIE));
        cout<<((win==square2win(g.gethuman()))?"you" : "PC")<<" won"<< '\n';
    }
    catch (const char *msg) {
        cerr << msg << '\n';
        getchar();
    }
    getchar();
}

board.h

#ifndef BOARD_H
#define BOARD_H
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <iostream>
const size_t NROW = 3;
const size_t NCOL = 3;

enum class square { EMPTY, X, O };
char square2char(const square &);
square char2square(char);

class board {
    square arr[NROW][NCOL];
public:
    board();
    board(const board &);
    board &operator=(const board &);
    friend std::ostream &operator<<(std::ostream &, const board &);
    void set(size_t, size_t, square);
    void set(size_t, size_t, char);
    square get(size_t, size_t) const;
};
#endif

board.cpp

#include "board.h"

char square2char(const square &s)
{
    switch (s) {
        case square::EMPTY:
            return ' ';
        case square::O:
            return 'O';
        case square::X:
            return 'X';
    }
}
square char2square(char c)
{
    switch (c) {
        case ' ':
        case '\0':
            return square::EMPTY;
        case 'X':
        case 'x':
            return square::X;
        case 'O':
        case 'o':
            return square::O;
    }
}

board::board()
{
    memset(this->arr, 0, sizeof arr);
}
board::board(const board &that)
{
    memcpy(this->arr, that.arr, sizeof(square) * NROW * NCOL);
}
board &board::operator=(const board &that)
{
    if (this != &that)
        memcpy(this->arr, that.arr, sizeof(square) * NROW * NCOL);
    return *this;
}
std::ostream &operator<<(std::ostream &os, const board &b)
{
    size_t i, j;

    for (i = 0; i < NCOL; ++i) {
        for (j = 0; j < NROW; ++j)
            os << '[' << square2char(b.arr[i][j]) << ']';
        putchar('\n');
    }
    return os;
}
void board::set(size_t zbrow, size_t zbcol, square s)
{
    if (zbrow >= NROW || zbcol >= NCOL || zbrow < 0 || zbcol < 0)
        throw "Out of range";
    if (this->arr[zbrow][zbcol] != square::EMPTY)
        throw "Square is full";

    this->arr[zbrow][zbcol] = s;
}
void board::set(size_t zbrow, size_t zbcol, char c)
{
    this->set(zbrow, zbcol, char2square(c));
}
square board::get(size_t zbrow, size_t zbcol) const
{
    return this->arr[zbrow][zbcol];
}

game.h

#ifndef GAME_H
#define GAME_H
#include "board.h"
enum class winstate { NOWIN, TIE, X, O };
winstate square2win(const square &);
square win2square(const winstate &);
class game {
    board *b;
    square human;
    square pc;
public:
    game();
    game(const game &);
    game &operator=(const game &);
    ~game();

    square gethuman() const;
    square getpc() const;

    void set(size_t, size_t);
    void pcset();
    friend std::ostream &operator<<(std::ostream &, const game &);
    winstate getwinner() const;
};
#endif

game.cpp

#include "game.h"
winstate square2win(const square &s)
{
    switch (s) {
        case square::EMPTY:
            return winstate::NOWIN;
        case square::O:
            return winstate::O;
        case square::X:
            return winstate::X;
    }
    return winstate::TIE;
}
square win2square(const winstate &w)
{
    switch (w) {
        case winstate::O:
            return square::O;
        case winstate::X:
            return square::X;
    }
    return square::EMPTY;
}
game::game()
{
    srand(time(NULL));

    this->b = new board;
    this->human = (square) (1 + rand() % 2);
    this->pc = (this->human == square::O) ? square::X : square::O;
}
game::game(const game &that)
{
    this->b = new board(*that.b);
    this->human = that.human;
    this->pc = that.pc;
}
game &game::operator=(const game &that)
{
    if (this != &that) {
        delete this->b;

        this->b = new board(*that.b);
        this->human = that.human;
        this->pc = that.pc;
    }
    return *this;
}
game::~game()
{
    delete this->b;
}
square game::gethuman() const
{
    return this->human;
}
square game::getpc() const
{
    return this->pc;
}
void game::set(size_t row, size_t col)
{
    this->b->set(row, col, this->human);
}
void game::pcset()
{
    size_t row, col;

    do {
        row = rand() % 3;
        col = rand() % 3;
    } while (this->b->get(row, col) != square::EMPTY);
    this->b->set(row, col, this->pc);
}
winstate game::getwinner() const
{
    size_t i, j;

    for (i = 0; i < NROW; ++i) {
        if (this->b->get(i, 0) != square::EMPTY
            && this->b->get(i, 0) == this->b->get(i, 1)
            && this->b->get(i, 0) == this->b->get(i, 2))
                return square2win(this->b->get(i, 0));
        if (this->b->get(0, i) != square::EMPTY
            && this->b->get(0, i) == this->b->get(1, i)
            && this->b->get(0, i) == this->b->get(2, i))
                return square2win(this->b->get(0, i));
    }
    if (this->b->get(0, 0) != square::EMPTY && 
              this->b->get(0, 0) == this->b->get(1, 1) && 
              this->b->get(0, 0) == this->b->get(2, 2))
        return square2win(this->b->get(0, 0));
    if (this->b->get(2, 0) != square::EMPTY 
              && this->b->get(2, 0) == this->b->get(1, 1) && 
              this->b->get(2, 0) == this->b->get(0, 2))
        return square2win(this->b->get(2, 0));

    for (i = 0; i < NROW; ++i)
        for (j = 0; j < NCOL; ++j)
            if (this->b->get(i, j) == square::EMPTY)
                return winstate::NOWIN;

    return winstate::TIE;
}
std::ostream &operator<<(std::ostream &os, const game &g)
{
    os << *(g.b);
    return os;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.