6
\$\begingroup\$

I've been working on a TicTacToe GUI. While the game works with a proper restart, exit and win conditions; I feel that it could still be better optimized.

I'm not particularly concerned with the cosmetics of the GUI. I'm aware of how to make an interface "pretty", so to speak. I would just like to know if there was a better way I could maybe check win conditions or set things up in general.

import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.*;

public class BasicGUI {
    private static String piece="O";
    protected static Boolean player=true;
    private static final JFrame frame = new JFrame("Tic Tac Toe");
    private static final JPanel panel=new JPanel(new GridLayout(4,3));
    protected static final JButton[] cells= new JButton[9];
    private static final JButton exitButton=new JButton("Exit");
    private static final JButton restartButton=new JButton("Restart");
    protected static final JLabel winLabel = new JLabel("Make a move");

    public static void main(String[] args){
         createWindow();
         createButtons();
    }


    //Set up frame
    private static void createWindow(){
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setSize(450, 600);
        frame.setVisible(true);
    }

    //Add action listeners to buttons
    private static void createButtons(){
        for(int i=0; i<9; i++){
            cells[i]=new JButton();
            cells[i].addActionListener(new ButtonHandler());
            panel.add(cells[i]);
        }
        exitButton.addActionListener(new ExitHandler());
        restartButton.addActionListener(new RestartHandler());
        panel.add(exitButton);
        panel.add(restartButton);
        panel.add(winLabel);
        frame.add(panel);
        frame.setVisible(true);
        //frame.pack();
    }

    protected static String getPiece(){
        return piece;
    }
    protected static void setPiece(String s){
        piece=s;
    }

    protected static Boolean checkWinner(){
        if(checkCells(cells[0], cells[4], cells[8])) return true;
        else if(checkCells(cells[2], cells[4], cells[6])) return true;

        else if(checkCells(cells[2], cells[5], cells[8])) return true;
        else if(checkCells(cells[1], cells[4], cells[7])) return true;
        else if(checkCells(cells[0], cells[3], cells[6])) return true;

        else if(checkCells(cells[0], cells[1], cells[2])) return true;
        else if(checkCells(cells[3], cells[4], cells[5])) return true;
        else if(checkCells(cells[6], cells[7], cells[8])) return true;
        else return false;
    }

    /*Checks the series in the array to see if they contain the same piece,
    * resulting in a win condition. */
    private static Boolean checkCells(JButton cell1, JButton cell2, JButton cell3){
        return(cell1.getText()==getPiece() &&
               cell2.getText()==getPiece() &&
               cell3.getText()==getPiece()
        );
    }
}

class ButtonHandler implements ActionListener{  
    public void actionPerformed(ActionEvent e){
        //Sets the button clicked to the piece then disables the button
        JButton pressedButton=(JButton)(e.getSource());
        pressedButton.setText(BasicGUI.getPiece());
        pressedButton.setEnabled(false);

        //If there is no winner; change the piece and player then exit method
        if(!BasicGUI.checkWinner()){
            BasicGUI.player=!BasicGUI.player;
            String changePiece=(BasicGUI.player)? "O":"X";
            BasicGUI.setPiece(changePiece);
            BasicGUI.winLabel.setText(BasicGUI.getPiece()+" make a move");
            return;
        }
        //If a win condition is found, disable all the buttons for the game an show who won
        for(JButton c:BasicGUI.cells){
            c.setEnabled(false);
            BasicGUI.winLabel.setText("Game over! "+BasicGUI.getPiece()+" won!");
        }
        return;
    }
}

//Exit game if clicked
class ExitHandler implements ActionListener{
    public void actionPerformed(ActionEvent e){
        System.exit(0);
    }
}

//Clear the text on the games buttons and re-enable them if disabled
class RestartHandler implements ActionListener{
    BasicGUI GUI = new BasicGUI();
    public void actionPerformed(ActionEvent e){
        for(JButton c:GUI.cells){
            c.setEnabled(true);
            c.setText("");
        }
    }
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$
  1. Improve the checkWinner method by introducing the concepts of rows, columns and diagonals. In this method the cells that belong to the same rows, columns and diagonals are checked. It is simple to calculate the indexes for cells that belong to a row or column by giving the row or column index: Cells in row r have indices r x row_length + i, where i runs from 0 to row_length-1. With this logic, it is easy to write a checkRow(r) method and use this in checkWinner(). This is less error prone, because there are less magic numbers in the code:
protected static Boolean checkWinner() {
  return 
    checkDiagonal(0) ||
    checkDiagonal(1) ||
    checkRow(0) ||
    checkRow(1) ||
    checkRow(2) ||
    checkColumn(0) ||
    checkColumn(1) ||
    checkColumn(2);
 } 

The Columns can be calculated equally simply. The Diagonals might need some thought.

  1. Multiple exits in the ButtonHandler. perform method. You use an if statement to check for a winner, but instead of using an else block, you return the whole method. Having multiple returns in a method makes it harder to read and to adjust later on, in case a common behavior is required after the if block.
  2. Incorrect documentation of checkCells method The documentation says it checks for the same value, where the implementation checks that they all have the value of the current player.
  3. Use of System.exit In general it is better to avoid this abrupt way of program termination to increase reusability. In this case, closing the window will halt the application.
  4. Reuse of ButtonHandler instance Each button has a separate instance of this handler, but since the handler has no state, this is not necessary. (Might be matter of taste)
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Some solid advice. May I ask why using System.exit is frowned upon in general? \$\endgroup\$ Commented Feb 11, 2015 at 22:43
6
\$\begingroup\$

A few points:

First of all, your formatting is inconsistent. You have stuff like:

protected static Boolean player=true;

And:

private static final JFrame frame = new JFrame("Tic Tac Toe");

Choose one and stick with it. I suggest the second option, as it is easier to read.

Your checkWinner() method looks nice, but if statements without braces makes me nervous. Try:

protected static Boolean checkWinner() {
    if (checkCells(cells[0], cells[4], cells[8])) { // Diagonal
        return true;
    } else if (checkCells(cells[2], cells[4], cells[6])) {
        return true;
    } else if (checkCells(cells[2], cells[5], cells[8])) { // Horizontal
        return true;
    } else if (checkCells(cells[1], cells[4], cells[7])) {
        return true;
    } else if (checkCells(cells[0], cells[3], cells[6])) {
        return true;
    } else if (checkCells(cells[0], cells[1], cells[2])) { // Vertical
        return true;
    } else if (checkCells(cells[3], cells[4], cells[5])) {
        return true;
    } else if (checkCells(cells[6], cells[7], cells[8])) {
        return true;
    }
    return false;
}

But that now wastes space. I would group everything together:

protected static Boolean checkWinner() {
    return checkCells(cells[0], cells[4], cells[8]) // Diagonal
            || checkCells(cells[2], cells[4], cells[6])
            || checkCells(cells[2], cells[5], cells[8]) // Vertical
            || checkCells(cells[1], cells[4], cells[7])
            || checkCells(cells[0], cells[3], cells[6])
            || checkCells(cells[0], cells[1], cells[2]) // Horizontal
            || checkCells(cells[3], cells[4], cells[5])
            || checkCells(cells[6], cells[7], cells[8]);
}

That looks better, and it makes me less nervous.


Probably a big problem is that your code does not handle ties. I will leave that to you, as it is pretty easy.

\$\endgroup\$
1
  • \$\begingroup\$ Didn't notice my inconsistency, thanks! I did have the braces in place beforehand but removed them to, like you say, avoid wasting space. The or operators are probably best used. I noticed the lack of a tie scenario pretty much as soon as I posted this -.- \$\endgroup\$ Commented Feb 11, 2015 at 22:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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