Conditionals validation for Tic Tac Toe

Question

I have been working with Java for a little more than a year. I recently have built a Tic Tac Toe game as an assignment for my Java class. After my instructor graded it, he wrote a comment around my validation method logic. Even though I got 100%, he said that the logic in my validation method is too cumbersome. He stated that I should look into a for orwhile` statement in order to clean out some code in my validation method.

Is there really a way to put all my conditional if statements in to a for or while loop? And if so, I would like to know what logic goes behind that. This program had a set of five arrays but in this validation method I worked only with the JButton array.

JButton [] button = new JButton [9];
public void validate()
    {
        if(button[0].getText().equals(button[1].getText()) && button[1].getText().equals(button[2].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[0].getText());
            gameOver();
            return;
        }
        else if(button[3].getText().equals(button[4].getText()) && button[4].getText().equals(button[5].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[3].getText());
            gameOver();
            return;
        }
        else if(button[6].getText().equals(button[7].getText()) && button[7].getText().equals(button[8].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[6].getText());
            gameOver();
            return;
        }
        else if(button[0].getText().equals(button[3].getText()) && button[3].getText().equals(button[6].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[0].getText());
            gameOver();
            return;
        }
        else if(button[1].getText().equals(button[4].getText()) && button[4].getText().equals(button[7].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[1].getText());
            gameOver();
            return;
        }
        else if(button[1].getText().equals(button[4].getText()) && button[4].getText().equals(button[7].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[1].getText());
            gameOver();
            return;
        }
        else if(button[2].getText().equals(button[5].getText()) && button[5].getText().equals(button[8].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[2].getText());
            gameOver();
            return;
        }
        else if(button[0].getText().equals(button[4].getText()) && button[4].getText().equals(button[8].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[0].getText());
            gameOver();
            return;
        }
        else if(button[2].getText().equals(button[4].getText()) && button[4].getText().equals(button[6].getText()))
        {
            JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[2].getText());
            gameOver();
            return;
        }

        int i;

        for(i=0;i<button.length;i++)
        {
            if(button[i].isEnabled())
            {
                break;
            }
        }


        if(i == button.length)
        {
            JOptionPane.showMessageDialog(null,"This was a Draw");
        }
    }

Answer 1

Start with replacing common code with function calls. For example, you can create a method for checking if an entire row has the same text:

bool checkRow(int row)
{
    int col = row*3;
    return button[col].getText().equals(button[col+1].getText())
           && button[col+1].getText().equals(button[col+2].getText());
}

Write similar methods for columns and diagonal. Then you can do something like that:

bool checkWin()
{
    for(int i=0; i<3; i++)
    {
        if(checkRow(i))
            return true;
        if(checkCol(i))
            return true;
    }
    if(checkMajorDiag())
        return true;
    if(checkMinorDiag())
        return true;
    return false;
}

And then your entire if-else chain will be replaced with:

if(checkWin())
    {
        JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[2].getText());
        gameOver();
        return;
    }

The code is now much more readable.

Two major points you'd want to remember:

1. Duplicate code leads to bugs

   Let's say you'd have to change the message printed to the user - when the message appears several times this task is tedious, plus in a real (big and complex) situation there's a good chance you'll forget to change one of the usages. In the new version, there's only one line to change.

2. Atomic operations are always better

   When I reviewed your code, I couldn't tell immediately what each if condition means.

   checkRow(0) means "check the first row". checkRow(0) is an atomic operation (atomic in this context means a single operation - a single function call).

   button[0].getText().equals(button[1].getText()) && button[1].getText().equals(button[2].getText()) is much less understandable. But the real danger here that if the line was button[0].getText().equals(button[1].getText()) && button[2].getText().equals(button[2].getText()), you probably couldn't tell that one index was changed. Which means that debugging atomic operations is much easier.

Answer 2

You have the button[1], button[4] and button[7] check twice in your code! This is a 'classic' mistake when copy/pasting blocks:

    else if(button[1].getText().equals(button[4].getText()) && button[4].getText().equals(button[7].getText()))
    {
        JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[1].getText());
        gameOver();
        return;
    }
    else if(button[1].getText().equals(button[4].getText()) && button[4].getText().equals(button[7].getText()))
    {
        JOptionPane.showMessageDialog(null,"Thank you the winner is" + button[1].getText());
        gameOver();
        return;
    }

This is a case where a preset table of values will be helpful to test your conditions.

Consider a structure which identifies what three buttons represent a winning condition:

private static final int[][] TRIPLES = {
    {0, 1, 2},
    {3, 4, 5},
    {6, 7, 8},

    {0, 3, 6},
    {1, 4, 7},
    {2, 5, 8},

    {0, 4, 8},
    {2, 4, 6}
}

This array represents the buttons that you check for a winning condition (except {1, 4, 7] is only in here once ;-) )

Now, with the above structure, consider the following loop:

for (int[] triple : TRIPLES) {
    int a = triple[0];
    int b = triple[1];
    int c = triple[2];
    if(    button[a].getText().equals(button[b].getText())
        && button[b].getText().equals(button[c].getText())) {

        JOptionPane.showMessageDialog(null,
              "Thank you the winner is" + button[a].getText());
        gameOver();
        return;

    }
}

This checks all your conditions, and there is very little code duplciation. If you want to, you can extract out the button-checks as suggested by palacsint, but I find the above is quite readable.

Answer 3

+1 to @opd and a few other notes:

1. You could extract out another method:

   private boolean buttonTextEquals(int index1, int index2) {
       return button[index1].getText().equals(button[index2].getText());
   }
   

   And use that in the checkRow():

   private boolean checkRow(int row) {
       int col = row * 3;
       return buttonTextEquals(col, col + 1) && buttonTextEquals(col + 1, col + 2);
   }
   

2. You could use a more descriptive name here than i:

   int i;
   

   What's the purpose of this variable? Use that as a name.

3. There were other Tic-tac-toe questions recently with great answer, you should check them.