A true beginner trying to learn - Tic-Tac-Toe-Trouble.

[Amelia]

Hi guys. A true beginner here. This is my tic-tac-toe program that allows two players to play competitively. It checks to see if either player has won, or if the board is filled completely. (This is a Practice Problem from Alex's book.)

The problem is - my program won't stop when it's a tie. It prints "It's a TIE.", and than immediately after that expects a move from Player 2. What am I doing wrong? (I'm aware that I could just add break statement and exit the loop immediately when tie_check==true, but I'm trying to learn.)

I apologize for bothering you with posting all of my code - I just hope it will give you a better picture of this situation.

#include <iostream>

using namespace std;

void drawBoard(string position[3][3])
{
    cout << "     0    1    2 \n";
    cout << "       |    |    \n";
    cout << "0   " << position[0][0] << "  | " << position[0][1] << "  | " << position[0][2] << "\n";
    cout << "   ____|____|____\n";
    cout << "       |    |    \n";
    cout << "1   " << position[1][0] << "  | " << position[1][1] << "  | " << position[1][2] << "\n";
    cout << "   ____|____|____\n";
    cout << "       |    |    \n";
    cout << "2   " << position[2][0] << "  | " << position[2][1] << "  | " << position[2][2] << "\n";
    cout << "       |    |    \n\n";
}

int checkForWinner (string position[3][3])

{
    /// check to see if X won the game
    //horizontal
    if (position[0][0]=="X" && position[0][1]=="X" && position[0][2]=="X")
    return 1;
    if (position[1][0]=="X" && position[1][1]=="X" && position[1][2]=="X")
    return 1;
    if (position[2][0]=="X" && position[2][1]=="X" && position[2][2]=="X")
    return 1;
    //vertical
    if (position[0][0]=="X" && position[1][0]=="X" && position[2][0]=="X")
    return 1;
    if (position[0][1]=="X" && position[1][1]=="X" && position[2][1]=="X")
    return 1;
    if (position[0][2]=="X" && position[1][2]=="X" && position[2][2]=="X")
    return 1;
    //diagonal
    if (position[0][0]=="X" && position[1][1]=="X" && position[2][2]=="X")
    return 1;
    if (position[0][2]=="X" && position[1][1]=="X" && position[2][0]=="X")
    return 1;

    /// check if O won
    //horizontal
    if (position[0][0]=="O" && position[0][1]=="O" && position[0][2]=="O")
    return 2;
    if (position[1][0]=="O" && position[1][1]=="O" && position[1][2]=="O")
    return 2;
    if (position[2][0]=="O" && position[2][1]=="O" && position[2][2]=="O")
    return 2;
    //  vertical
    if (position[0][0]=="O" && position[1][0]=="O" && position[2][0]=="O")
    return 2;
    if (position[0][1]=="O" && position[1][1]=="O" && position[2][1]=="O")
    return 2;
    if (position[0][2]=="O" && position[1][2]=="O" && position[2][2]=="O")
    return 2;

    // diagonal

    if (position[0][0]=="O" && position[1][1]=="O" && position[2][2]=="O")
    return 2;
    if (position[0][2]=="O" && position[1][1]=="O" && position[2][0]=="O")
    return 2;

}

bool checkForTie ( string position[3][3])
{
    bool tie;
    int non_empty;
    non_empty=0;
    for (int i=0; i<3; i++)
    {
        for (int j=0; j<3; j++)
        {
            if (position[i][j]!=" ")
            {
                non_empty++;
            }
        }
    }
    if (non_empty==9)
    {
        tie=true;
    }
    else
    {
        tie=false;
    }
    return tie;

}

int main()
{

    bool winner;
    int win_check;
    bool tie_check;
    string position [3][3];
    for (int i=0; i<3; i++)
    {
        for (int j=0; j<3; j++)
        {
            position[i][j]=" ";
        }
    }

    drawBoard(position);

    do
    {
        int i;
        int j;
        cout<<"Player 1, your move. \n";
        cout<<"Row?\n";

        cin>>i;
        cout<<"Column?\n";
        cin>>j;
        position[i][j]="X";
        drawBoard(position);
        win_check=checkForWinner(position);
        if (win_check==1)
        {
            winner=true;
            cout<<"We have a WINNER. Player 1 WON!\n";

        }
        else
        {
            winner=false;
        }
        tie_check=checkForTie(position);
        if (tie_check==true)
        {
            cout<<"It's a TIE.";

        }
        cout<<"Player 2, your move. \n";
        cout<<"Row?";
        cin>>i;
        cout<<"Column?\n";
        cin>>j;
        position[i][j]="O";
        drawBoard(position);
        win_check=checkForWinner(position);
        
        if (win_check==2)
        {
            winner=true;
            cout<<"We have a WINNER. Player 2 WON!\n";
        }
        else
        {
            winner=false;
        }
        
        tie_check=checkForTie(position);
        
        if (tie_check==true)
        {
            cout<<"It's a TIE.";
        }

    } while (winner!=true && tie_check!=true);
}

One more thing. This was my first version of a function that checks if the board is filled completely. The idea was:

for each element in the array
if element==" ";
return false immediately;

//... and if this whole loop completes without detecting false, then return true;

Here's the code:

bool checkForTie (string position [3][3])
{ bool tie;
    for (int i=0; i<3; i++)
    {
        for (int j=0; j<3; j++)
        {
            if (position[i][j]==" ")
            {
                tie=false;
                break;
            }

        }

    }
    if (tie!=false)
    {
        tie=true;
    }

    return tie;
}

It never detected a tie. Why?

Sorry for the super-long post. Any help is much appreciated.

[dandrestor]

I haven't fully read your source code, but at first glance I see that you have a lot of code duplication in your main do/while game loop.
I would do something like:

bool over = false;
int current_player = 2;
do
{
if (current_player == 1) current_player = 2; else current_player = 1;
getMove(current_player);
over = checkFinished();
} while (!over)

And take the duplicated code out into getMove() and checkFinished() - which checks and prints messages for ties and wins.

[dandrestor]

break will only break out of the innermost for loop. Try return instead.
Also, make sure you initialize your variable. This will also eliminate the need for that last "if" statement (which has no effect anyway).

bool checkForTie (string position [3][3])
{ bool tie = true; //always initialize your variables!
    for (int i=0; i<3; i++)
    {
        for (int j=0; j<3; j++)
        {
            if (position[i][j]==" ")
            {
                return false; // This will automatically exit the function
            }

        }

    }
/* tie is bool, so the following if has virtually no effect.
    if (tie!=false)
    {
        tie=true;
    }
*/

    return tie;
}

[Ewiv]

int checkForWinner (string position[3][3])

Has a lot of repeating code. Any time you start seeing repeating code you want to look for ways of turning the repeating code into a function to reduce the amount of code written and tidy up your program. In this case you could have the function take a 2nd variable indicating the player to check a win for and then simply check for a win in your game loop. Note that in that case you could change the return to a bool and then report who won based on who just moved when it returns true.

And as the previous poster mentioned, always initiate your variables or be prepared to see unexpected behavior or crashes in your programs.

[hk_mp5kpdw]

You make no check that a move is valid. You need something that checks if the row/column is available to be played (is currently empty) before you go ahead and put an X/O at that position otherwise there is nothing stopping someone from specifying a position that is already occupied by the other player and overwriting it with their own marker.

[MutantJohn]

I think this is the perfect time to learn object oriented programming, imo. Unless maybe you're a true true beginner and it might be too much otherwise a lesson in encapsulation might be worthwhile.

[Amelia]

Thank you so much for your comments.

I haven't fully read your source code, but at first glance I see that you have a lot of code duplication in your main do/while game loop.
I would do something like:

And take the duplicated code out into getMove() and checkFinished() - which checks and prints messages for ties and wins.

I've reconstructed my code following your advice and it works! Yaaay! Thanks!

Note that in that case you could change the return to a bool and then report who won based on who just moved when it returns true.

And as the previous poster mentioned, always initiate your variables or be prepared to see unexpected behavior or crashes in your programs.

This is exactly what I've done and the world is much better place right now, when my code works (and looks prettier,too. Let's not forget that.)! The funny thing is... I have a neon sticky-note on my monitor that says ALWAYS INITIALIZE YOUR VARIABLES. And yet I failed so miserably by forgetting to do so.

You make no check that a move is valid.

Yes, I'm aware of that. This was just a practice problem from a book - and as funny as it may sound to you, this problem was pretty challenging for me. Although I'll make sure to implement that method soon.

I think this is the perfect time to learn object oriented programming, imo. Unless maybe you're a true true beginner and it might be too much otherwise a lesson in encapsulation might be worthwhile.

True true beginner. My "Hello world" happened two months ago. But I've read something on the subject and it seems quite handy. I'll get there eventually.

Once again, thank you all so much.

[Ewiv]

If you want, post op your current code and we'll help you refine it more.

Otherwise congrats on your conquest and best of luck on climbing the next mountain in your learning process

[dandrestor]

Glad you could make it work. There are many good habits that you should learn, especially if you're just starting programming. Search on the net, there's a ton of good literature out there. Initializing variables is just one of these.

Another is keeping your functions short - and focused. Decompose your problem well, and make sure you assign to each function just one responsibility, and that the function does just that, and does it well. Very long functions are often a sign of bad design.

Cheers,
Dan

[MutantJohn]

I'd disagree with that, actually. Short functions, long functions, who cares?

Instead, decide when to break up your code into smaller functions to avoid repetitive code. If you're going to be doing the same kind of routine over and over again, then it might be a good idea to break the code up. Otherwise, you're breaking up the natural flow of the code and make it harder to read because instead of following the code that I want to, I have to now look up where the other function is and what it does and what it would do in the context of the code I'm analyzing.

I say usage, not length.

[laserlight]

Short functions, long functions, who cares?

When you have to debug a 900+ line function -- one that most certainly does not have just one responsibility -- because of such an attitude, you will care. (No, this is neither an exaggeration nor is it merely a hypothetical scenario.)

decide when to break up your code into smaller functions to avoid repetitive code. If you're going to be doing the same kind of routine over and over again, then it might be a good idea to break the code up.

That is good advice, and it is part of what "decompose your problem well" means. But a function may have an overall responsibility that is logically separated into different parts, none of which are repeated. If expressed as one big chunk, the function would be doing many different things under that scope of responsibility. This makes it harder to understand since you have to examine the entire function as a whole to understand it. Properly testing a monolithic chunk is also more difficult as it is more likely that necessary test cases will be missed.

Otherwise, you're breaking up the natural flow of the code and make it harder to read because instead of following the code that I want to, I have to now look up where the other function is and what it does and what it would do in the context of the code I'm analyzing.

If a large function was appropriately broken up into well named functions, then that is a non-problem. With smaller chunks, a reader can understand the function more easily since there are fewer things happening in the current scope.

EDIT:

bool checkForTie (string position [3][3])
{ bool tie = true; //always initialize your variables!
    for (int i=0; i<3; i++)
    {
        for (int j=0; j<3; j++)
        {
            if (position[i][j]==" ")
            {
                return false; // This will automatically exit the function
            }
 
        }
 
    }
/* tie is bool, so the following if has virtually no effect.
    if (tie!=false)
    {
        tie=true;
    }
*/
 
    return tie;
}

If you're going to return from within the nested for loops, then there is no point having the tie variable. You might as well simplify to:

bool checkForTie(string position[3][3])
{
    for (int i = 0; i < 3; ++i)
    {
        for (int j = 0; j < 3; ++j)
        {
            if (position[i][j] == " ")
            {
                return false;
            }
        }
    }
    return true;
}

[dandrestor]

If a large function was appropriately broken up into well named functions, then that is a non-problem. With smaller chunks, a reader can understand the function more easily since there are fewer things happening in the current scope.

Laserlight is absolutely right. While minimizing function length is not a goal in itself, I think decomposing the problem into manageable, non-repetitive and easily debuggable chunks will lead to short(er) functions.

[MutantJohn]

I like your logic, laser, but 900 lines of debugging is 900 lines of debugging, no matter how you approach it. I get that partitioning functions is a different way to approach the problem but if your code is spread out over that many lines, it's going to be hard to debug.

I think it's a matter of personal preference, in that case.

However, I will still maintain that using functions to avoid repetitive code is devoid of personal preference and should always be done, if the problem can't be avoided by caching (I did it, guys; I'm not talking about the CPU cache this time).

[dandrestor]

I like your logic, laser, but 900 lines of debugging is 900 lines of debugging, no matter how you approach it.

I disagree, debugging 900 lines of code sharing the same state is totally different than debugging 90 chunks of 10 lines, each with its own state, preconditions, postconditions and invariants.

[laserlight]

I like your logic, laser, but 900 lines of debugging is 900 lines of debugging, no matter how you approach it. I get that partitioning functions is a different way to approach the problem but if your code is spread out over that many lines, it's going to be hard to debug.

If the function had been properly broken up, then it isn't 900 lines of debugging at once. It is 30 lines here, 50 lines there, 70 lines in another place. I know that the 30 line function works as intended. Move on. I know that the 50 line function works as intended. Move on. 70 lines? Right on it.

With a scope that was 900+ lines long, I had a hard time because I had to be very wary of the possibility a change in one place would affect somewhere else that I did not expect. Effectively, despite being local to the function, the variables were "global", hence the harmful aspects of global variables came into play.