C++ console TicTacToe: Checking Win Conditions

Question

The game board is stored as a 2D char array. The Player moves his cursor around the board using the numpad, and chooses with the enter key- current position of the cursor is stored in two ints.

After each move, the board is evaluated for a win using the below method.

void checkwin()
{
    //look along lines from current position
    int x = cursorPosX;
    int y = cursorPosY;
    int c = playerTurn ? 1 : 2; //which mark to look for

    for (int xAxis = 0; xAxis <= 2; xAxis++) //look along x axis
    {
        x = WrapValue(0, sizeof(squares[0]), x + 1);
        if (CheckPos(x, y) != c) //if we don't find the same mark, must not be a horizontal line, otherwise, break out.
        {
            x = cursorPosX; //reset x
            for (int yAxis = 0; yAxis <= 2; yAxis++) //look along y axis
            {
                y = WrapValue(0, sizeof(squares[0]), y + 1);
                if (CheckPos(x, y) != c) 
                {
                    y = cursorPosY;
                    //look for diagonal
                    for (int i = 0; i <= 2; i++ )
                    {
                        x = WrapValue(0, sizeof(squares[0]), x + 1);
                        y = WrapValue(0, sizeof(squares[0]), y + 1);
                        if (CheckPos(x, y) != c)
                        {
                            //failed everything, return
                            winConditions = -1;
                            return;
                        }
                    }
                    break;
                }
            }
            break;
        }
    }
    //if we make it out of the loops, we have a winner.
    winConditions = playerTurn ? 0 : 1;
}

I get wrong results- returning a draw or win when not appropriate. I'm almost certain x and y get wrong values at some point and start checking the wrong spots.

Visual Studio stops updating a watch on x and y after going into the yAxis loop- I'm not sure why, but it prevents me from keeping track of those values. Am I breaking a rule about scoping somewhere? This is the only place I use x and y as variable names.

Relevant wrap method below. My aim was to always be able to check the other 2 spaces by adding, no matter where I was on the board

int WrapValue(int min, int max, int value)
{
    auto range = max - min;

    while (value >= max)
    {
        value -= range;
    }
    while (value < min)
    {
        value += range;
    }

    return value;
}

I'd appreciate a trained eye to tell me what I did wrong here. Thanks so much for your time.

Do you know how to debug? If not: this is the perfect time to learn it. — Gewure, Dec 12 '16 at 17:04
I suggest: 1) Compile the program with debug information; 2) Start the program in a debugger; 3) Use the *next* statement command to execute the next statement and view variable values; 4) Step *into* functions and see how they operate. Search the internet for "c++ how to debug small programs". — Thomas Matthews, Dec 12 '16 at 17:12
side note, in your WrapValue method if `value > 2*max` it won't be caught. same for min. You could turn it into `auto range = max-min; while(value >= max) value-=range; while(value — RyanP, Dec 12 '16 at 17:14
Look up a short and juicy tutorial into debugging with your IDE of choice. Set a breakpoint (i think ONE single breakpoint at your "return value;" line should be sufficient - then start the debugging. That is: let your IDE walk through each line one by one - and watch the values in your variables. This way you will find out where your bug resides. If you look at code and go through it line-by-line in your head, you are doing human-debugging. its not complicated at all, once you get it. Also it is a very important skill for a programmer. — Gewure, Dec 12 '16 at 17:14
@ThomasMatthews, -As I mentioned, when I step through, x and y go stale when I enter the YAxis for loop. Would you happen to know why? — Luke, Dec 12 '16 at 17:22
Edited to reflect @RyanP 's better implementation. Tested, problem still exists. — Luke, Dec 12 '16 at 17:40
This code is way too complicated for me to understand. Instead of all that deeply nested stuff, make three separate checks: is there a horizontal win? is there a vertical win? is there a diagonal win? — Pete Becker, Dec 12 '16 at 18:17
@PeteBecker LOL that's what I'm doing now. Makes sense to refactor into 1 method that checks along a specified line called multiple times. — Luke, Dec 12 '16 at 18:25
@Luke you'd have an easier time making `checkwin()` take a const reference to the board as an argument. That would be a better design, and it would make it easier for us to see what the code is doing. `int x = cursorPosX;` I don't know what this means because I can't see when checkwin is called, how cursorPosX is being manipulated, etc. If the code checked a board passed in for a win and returned the result, we could see if the function was correct by inspection instead of needing to see what the rest of the code does. — RyanP, Dec 12 '16 at 19:21

score 1 · Answer 1 · answered Dec 12 '16 at 20:08

1

Nesting for loops was a terrible idea. I solved the problem by refactoring the code into multiple separate loops that each do 1 thing, rather than fall through each other into deeper levels of hell.

for (int xAxis = 0; xAxis <= 2; xAxis++) //look along x axis
{
    x = WrapValue(0, sizeof(squares[0]), x + 1);
    if (CheckPos(x, y) != c) //if we don't find the same mark, must not be a horizontal line, otherwise, break out.
    {
        x = cursorPosX; //reset x
        break;
    }
    else if (xAxis == 2)
    {
        winConditions = playerTurn ? 0 : 1;
        return;
    }
}

for (int yAxis = 0; yAxis <= 2; yAxis++) //look along y axis
{
    y = WrapValue(0, sizeof(squares[0]), y + 1);
    if (CheckPos(x, y) != c)
    {
        y = cursorPosY;
        break;
    }
    else if (yAxis == 2)
    {
        winConditions = playerTurn ? 0 : 1;
        return;
    }
}
...ect

This violates DRY, but it does work the way it's supposed to, I'm sure I can simplify it later.

While I'm not entirely sure why the previous way didn't work, I do realize that it was just bad design to start with.

answered Dec 12 '16 at 20:08

Luke

86
4

I'll have to remember to accept in 2 days, but I think this question can be closed now, as the problem has been resolved. – Luke Dec 12 '16 at 20:10
The reason it fixed your problem is that you were declaring new x and y in the inner for loops that was masking the outer definitions. Although it is legal code your compiler should have warned you. Are you ignoring warnings? – stark Dec 12 '16 at 22:30
I always listen to warnings, whether from my mother or my compiler. So you're saying when I assign a value to a variable initialized outside of a loop, it initializes a new variable with the same name? Even though I never initialized explicitly? I wonder if you could link me to something that expounds upon this? – Luke Dec 12 '16 at 23:45
DRY is good design :) ... but in C its sometimes the good design which makes the bad design. – Gewure Dec 13 '16 at 15:25

C++ console TicTacToe: Checking Win Conditions

1 Answers1