Tic Tac Toe game

Question

I don't have any questions about this piece of code; it compiles just fine in Visual Studio 2010. I just wanted to post it to see if anyone has any good ideas on how to simplify it, or pointing out any bad practices I have have done.

// Tic Tac Toe Game
#include <iostream>
#include <iomanip>
using namespace std;

class TicTacToe
{
private:
        char Board[3][3];
    int play();
    void playermove();
    int turn(int);
    int check (int, int);
    void boardProgress(char x[][3]);
    int Winner(char x[][3]);
    int player1, player2, draw, player,winner,done;
    int row, col;

public:
    int TG, T1, T2, TD;
    void intBoard();
    int Stats(int);

    char playAgain(char);
}playBall;

int main()
{
    playBall.TG = 0,playBall.T1 = 0,playBall.T2 = 0,playBall.TD = 0;
    int Winner = 0;
        // This will only display when the program is first run
        cout << "You, are about to witness the program of the century.\n\n";

    char done = false;

    while (!done)
        {
        // This will display before each game
        cout << "COMMENCE TIC TAC TOE!!!!\n\n"
                "Ladies first, please pick the row then the column (1 through 3).\n\n";

        // Play a game and remember who won (if anyone)
        playBall.intBoard();

    //If you want to play again or not
    done = playBall.playAgain(done);
}

// Finish
cout << "\n\nThank you for playing!\n";
system ("pause");
return 0;
}
void TicTacToe::intBoard()
{
    done = false;
    winner = 0;
    player = 2;
    player1 = 1;
    player2 = 2;
    draw = 3;
    int i,k;
    for (k=0;k<3;k++)
    {
        for (i=0;i<3;i++)
        {
            Board[k][i]=0;
        }
    }
    play();
}

int TicTacToe::play()
{
    int done = false;
    boardProgress(Board);
    while(!done)
    {
        playermove();
        boardProgress(Board);
        done = Winner(Board);
    }
    Stats(winner);
    return 0;
}
void TicTacToe::playermove()
{
    int answer = false;
    while (answer == false)
    {
        cout <<"Row: ";
        cin >> row;
        row--;
        cout <<"Colume: ";
        cin >> col;
        col--;
        answer = check(row,col);
    }
    player = turn(player);
    if (player == 1)
        Board[row][col] = 'X';
    else if (player == 2)
        Board[row][col] = 'O';
    else
        cout<< "Failed.";
}
int TicTacToe::check(int row, int col)
{
    if(Board[row][col] == 0)
    {
        return true;
    }
    else if (Board[row][col] != 0)
    {
        cout <<"Can go there, Dude!\n\n";
        return false;
    }
    else
    {
        cout<< "Going through Check";
        Winner(Board);
    }
    return false;
}

int TicTacToe::turn(int player)
{
    switch(player)
    {
    case 1: player = 2;
        {
        cout<<"\nPlayers 1 turn.\n\n";
        break;
        }
    case 2: player = 1;
        {
        cout<<"\nPlayers 2 turn.\n\n";
        break;
        }
    }
    return player;
}

int TicTacToe::Winner(char x[][3])
{
    winner = 0;
    int count = 0;
    int a = row;
    int b = col;
    for (a=0;a<3;a++)
    {
        for (b=0;b<3;b++)
        {
            if (Board[a][b]==0)
            {
                count++;
            }
        }
    }
    if (count > 0)
    {
        int row, col, r, c, d, ro, co, dO;
        for (row=0; row<3; row++)
        {
            r=0;
            ro=0;

            for (col=0; col<3; col++)
            {
                if(x[row][col]=='X')
                    r++;
                if(x[row][col]=='O')
                    ro++;
                if (ro==3)
                { 
                    winner=2;
                }
                if (r==3)
                {   
                    winner=1;
                }
            }
        }
        for (col=0; col<3; col++)
        {
            r=0;
            ro=0;
            for (row=0; row<3; row++)
            {
                    if(x[row][col]=='X')
                    r++;
                if(x[row][col]=='O')
                    ro++;
                if (ro==3)
                {   
                    winner=2;
                }
                if (r==3)
                {
                    winner=1;
                    }
            }
        }
        if (x[0][0]=='X' && x[1][1]=='X' && x[2][2]=='X')
        {
            winner=1;
        }
        else if (x[0][0]=='O' && x[1][1]=='O' && x[2][2]=='O')
        {
            winner=2;
        }
        else if (x[2][0]=='X' && x[1][1]=='X' && x[0][2]=='X')
        {
            winner=1;
        }
        else if (x[2][0]=='O' && x[1][1]=='O' && x[0][2]=='O')
        {
            winner=2;
        }
    }
    else if (count == 9)
    {
        cout << "Its a draw, Bummer.";
        winner = 3;
    }   
    else
    {
        cout<< "Next Player go, MEOW!\n\n";
    }
    if (winner > 0)
    {
        done = true;
    }
return done;
}

int TicTacToe::Stats(int winner)    
{
    playBall.TG++;
    switch(winner)
    {
    case 1: T1++;
        {
        break;
        }
    case 2: T2++;
        {
        break;
        }
    case 3: TD++;
        {
        break;
        }
    }
    cout<<"You have played this many games: " << TG;
    cout<<"\nPlayer1 won has won: " << T1 << " games."; 
    cout<<"\nPlayer2 won has won: " << T2 << " games.";
    cout<<"\nYou have had: " << TD << " draws.";
    return 0;
}

char TicTacToe::playAgain(char done)
{
    cout <<"\n\nWanna play again!?\n\n";
    cout <<"Y/N: ";
    cin >> done;
    if(done == 'Y' || done == 'y')
    {
         done = false;
    }
    else
    {
        done = true;
    }
    return done;
}
void TicTacToe::boardProgress(char x[][3])
{
    cout << "      |     |     \n";
    cout << "   " << x[0][0] << "  |  " << x[0][1] << "  |  " << x[0][2] <<"  \n";
    cout << " _____|_____|_____\n";
    cout << "      |     |     \n";
    cout << "   " << x[1][0] << "  |  " << x[1][1] << "  |  " << x[1][2] <<"  \n";
    cout << " _____|_____|_____\n";
    cout << "      |     |     \n";
    cout << "   " << x[2][0] << "  |  " << x[2][1] << "  | " << x[2][2] <<"  \n";
    cout << "      |     |     \n";
}

Answer 1

Well for one, you've got some unreachable code:

if(Board[row][col] == 0)
{
    return true;
}
else if (Board[row][col] != 0)
{
    cout <<"Can go there, Dude!\n\n";
    return false;
}
else
{
    // The cell is neither 0 nor non-0!!!
    // cout << "What the hell, man?";
    cout << "Going through Check";
    Winner(Board);
}
return false;

Since you're using C++, you should create a Board class that houses the cells and the win check routine. Your class should look like this:

enum Piece {NONE, X, O};

public class Board {
private:
    Piece cells[3][3];

public:
    void put(int x, int y, Piece piece);
    Piece get(int x, int y);
    Piece checkWin();
    void clear();
}

Answer 2

• Do not pollute your namespace by using namespace std .
• winner = 1 (2, 3) smells bad, using nice constant or enum would improve readability. Same with board containing 0s and 1s. Do not force your reader to guess / scroll to get that knowledge.
• Your implementation is tightly coupled with console - what forbids you to implement abstract logic encapsulated in class and USE this class in console app? What if you want to port it to GUI app? Can you do it cheaply now?
• Winner()? Capitalized letter means class. Always. Do not misguide your reader. Same thing with Stats().
• boardProgress? Hell, how's that better than just printBoard()?
• TG? T1? T2? T3? gamesPlayed, gamesWonByPlayer1, gamesWonByPlayer2, gamedDrawn (is it correct form in English?) are a little better I think.
• 3 is a magic number, and it's all over the place. Make it a constant or pass it to constructor, so playing on 4x4 or 5x5 board is also available. Logic won't change.
• As always, multiple indentation is a big 'let's get away from it'. Testing is nightmare, debugging is nightmare, understanding it is nightmare. According to Uncle Bob, more than one indentation is too much. I'm not that radical, but certainly > 2 indentation levels and > 20 lines are too much (see the Winner() method).

Answer 3

Don't use system("pause");; this is platform-specific. You might even cause a nuclear meltdown.

using namespace std; is bad practice. Read this

Why is play() returning an int? You’re not using the returned value, so make it a void function.

check() is expected to return an int, yet you return true/false. If someone is reading your code, he/she expects to see an int. Otherwise, that programmer will have to spend more time figuring out what’s happening in the function. If there isn't a compatibility issue with C, have the function return bool instead. Same goes for the Winner() function.

player = turn(player)?

Some unnecessary copying here. Pass a reference of the variable player instead, since you are only checking the value of it.

Answer 4

Your winner function can (should) be reduced:

if (x[0][0]=='X' && x[1][1]=='X' && x[2][2]=='X')
{
    winner=1;
}
else if (x[0][0]=='O' && x[1][1]=='O' && x[2][2]=='O')
{
    winner=2;
}
else if (x[2][0]=='X' && x[1][1]=='X' && x[0][2]=='X')
{
    winner=1;
}
else if (x[2][0]=='O' && x[1][1]=='O' && x[0][2]=='O')
{
    winner=2;
}

First, like Eric said, you need a board class (or game class makes more sense) to manage players with something like:

game.getCurrentPlayer()
game.setCurrentPlayer()

When a game starts: game.setCurrentPlayer('X') //(or 'O') and do that after each move.

Then in your winner function you can use this to reduce the code by half:

player = game.getCurrentPlayer();
if (x[0][0]==player && x[1][1]==player && x[2][2]==player)
   winner = player;
//...

You also use for loops to check rows and columns. Just hardcode everything like you did with the diagonal check. You get 8 conditions:

if (x[0][0]==player && x[0][1]==player && x[0][2]==player) // first row

For a small game like TTT you don't need loops to check rows, columns and diagonals, it makes the code hard to read (especially with variables like r, ro, row...etc.)