Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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");
        }
    }
share|improve this question

3 Answers 3

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.

share|improve this answer
    
But you always show button[2].getText() as the winner! –  Florian F Sep 25 at 15:59

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.

share|improve this answer

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.