Conditionals validation for Tic Tac Toe

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()))
{

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:

-
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.

-
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.

StackExchange Code Review Q#45097, answer score: 8