3
\$\begingroup\$

I have made a simple Tic Tac Toe game but I think I have not followed a few conventions. I hope if you could help me improve it by suggesting some fixes.

package practice;
import javax.swing.*;
import java.awt.*;
import java.awt.event.*;


public class TicTacToe extends JFrame implements ActionListener{

    /**
     * 
     */
    private static final long serialVersionUID = 1L;
    private JPanel panel;
    private JButton button1,button2,button3,button4,button5,button6,button7,button8,button9;
    private String letter = "";
    private boolean win = false;
    private int count = 0;

    public static void main(String[] args){
        new TicTacToe();
    }

    public TicTacToe(){
        this.setSize(500,500);
        this.setTitle("Tic Tac Toe");
        this.setDefaultCloseOperation(EXIT_ON_CLOSE);
        this.setLocationRelativeTo(null);
        this.setResizable(false);


        button1 = new JButton("");
        button2 = new JButton("");
        button3 = new JButton("");
        button4 = new JButton("");
        button5 = new JButton("");
        button6 = new JButton("");
        button7 = new JButton("");
        button8 = new JButton("");
        button9 = new JButton("");


        panel = new JPanel();
        panel.setLayout(new GridLayout(3,3));

        panel.add(button1);
        panel.add(button2);
        panel.add(button3);
        panel.add(button4);
        panel.add(button5);
        panel.add(button6);
        panel.add(button7);
        panel.add(button8);
        panel.add(button9);

        button1.addActionListener(this);
        button2.addActionListener(this);
        button3.addActionListener(this);
        button4.addActionListener(this);
        button5.addActionListener(this);
        button6.addActionListener(this);
        button7.addActionListener(this);
        button8.addActionListener(this);
        button9.addActionListener(this);



        this.add(panel);
        this.setVisible(true);
    }

    public void actionPerformed(ActionEvent e){
        count++;
        if(count%2==1){
            letter = "X";
        }
        else if(count %2==0){
            letter = "O";
        }

        if(e.getSource()==button1){
            button1.setText(letter);
            if(letter == "X"){
            button1.setIcon(new ImageIcon("X.png"));
            button1.setDisabledIcon(new ImageIcon("X.png"));
            }
            else{
                button1.setIcon(new ImageIcon("O.png"));
                button1.setDisabledIcon(new ImageIcon("O.png"));
            }
            button1.setEnabled(false);
        }
        if(e.getSource()==button2){
            button2.setText(letter);
            if(letter == "X"){
                button2.setIcon(new ImageIcon("X.png"));
                button2.setDisabledIcon(new ImageIcon("X.png"));
                }
                else{
                    button2.setIcon(new ImageIcon("O.png"));
                    button2.setDisabledIcon(new ImageIcon("O.png"));
                }
            button2.setEnabled(false);
        }
        if(e.getSource()==button3){
            button3.setText(letter);
            if(letter == "X"){
                button3.setIcon(new ImageIcon("X.png"));
                button3.setDisabledIcon(new ImageIcon("X.png"));
                }
                else{
                    button3.setIcon(new ImageIcon("O.png"));
                    button3.setDisabledIcon(new ImageIcon("O.png"));
                }
            button3.setEnabled(false);
        }
        if(e.getSource()==button4){
            button4.setText(letter);
            if(letter == "X"){
                button4.setIcon(new ImageIcon("X.png"));
                button4.setDisabledIcon(new ImageIcon("X.png"));
                }
                else{
                    button4.setIcon(new ImageIcon("O.png"));
                    button4.setDisabledIcon(new ImageIcon("O.png"));
                }
            button4.setEnabled(false);
        }
        if(e.getSource()==button5){
            button5.setText(letter);
            if(letter == "X"){
                button5.setIcon(new ImageIcon("X.png"));
                button5.setDisabledIcon(new ImageIcon("X.png"));
                }
                else{
                    button5.setIcon(new ImageIcon("O.png"));
                    button5.setDisabledIcon(new ImageIcon("O.png"));
                }
            button5.setEnabled(false);
        }
        if(e.getSource()==button6){
            button6.setText(letter);
            if(letter == "X"){
                button6.setIcon(new ImageIcon("X.png"));
                button6.setDisabledIcon(new ImageIcon("X.png"));
                }
                else{
                    button6.setIcon(new ImageIcon("O.png"));
                    button6.setDisabledIcon(new ImageIcon("O.png"));
                }
            button6.setEnabled(false);
        }
        if(e.getSource()==button7){
            button7.setText(letter);
            if(letter == "X"){
                button7.setIcon(new ImageIcon("X.png"));
                button7.setDisabledIcon(new ImageIcon("X.png"));
                }
                else{
                    button7.setIcon(new ImageIcon("O.png"));
                    button7.setDisabledIcon(new ImageIcon("O.png"));
                }
            button7.setEnabled(false);
        }
        if(e.getSource()==button8){
            button8.setText(letter);
            if(letter == "X"){
                button8.setIcon(new ImageIcon("X.png"));
                button8.setDisabledIcon(new ImageIcon("X.png"));
                }
                else{
                    button8.setIcon(new ImageIcon("O.png"));
                    button8.setDisabledIcon(new ImageIcon("O.png"));
                }
            button8.setEnabled(false);
        }
        if(e.getSource()==button9){
            button9.setText(letter);
            if(letter == "X"){
                button9.setIcon(new ImageIcon("X.png"));
                button9.setDisabledIcon(new ImageIcon("X.png"));
                }
                else{
                    button9.setIcon(new ImageIcon("O.png"));
                    button9.setDisabledIcon(new ImageIcon("O.png"));
                }
            button9.setEnabled(false);
        }
        //horizontal wins
        if(button1.getText() == button2.getText() && button2.getText()== button3.getText() && button1.getText()!=""){
            win = true;
        }
        else if(button4.getText() == button5.getText() && button5.getText()== button6.getText() && button4.getText()!=""){
            win = true;
        }
        else if(button7.getText() == button8.getText() && button8.getText()== button9.getText() && button7.getText()!=""){
            win = true;
        }
        //vertical wins
        else if(button1.getText() == button4.getText() && button4.getText()== button7.getText() && button1.getText()!=""){
            win = true;
        }
        else if(button2.getText() == button5.getText() && button5.getText()== button8.getText() && button2.getText()!=""){
            win = true;
        }
        else if(button3.getText() == button6.getText() && button6.getText()== button9.getText() && button3.getText()!=""){
            win = true;
        }
        //diagonal wins
        else if(button1.getText() == button5.getText() && button5.getText()== button9.getText() && button1.getText()!=""){
            win = true;
        }
        else if(button3.getText() == button5.getText() && button5.getText()== button7.getText() && button3.getText()!=""){
            win = true;
        }
        else{
            win = false;
        }


        if(win == true){
            JOptionPane.showMessageDialog(null,letter+" = wins");
            System.exit(0);
        }
        else if(count == 9 && win== false){
            JOptionPane.showMessageDialog(null,"Tie Game");
            System.exit(0);
        }

    }

}
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to CodeReview, I hope you get some fine answers. \$\endgroup\$
    – Legato
    Nov 20, 2015 at 17:07
  • \$\begingroup\$ In addition to @Legato's answer: - Get rid of Swing, switch to JavaFX. Swing is no longer recommended by Oracle, and JavaFX is the alternative recommended by Oracle. \$\endgroup\$ Nov 20, 2015 at 18:08

1 Answer 1

4
\$\begingroup\$

Use arrays

The first thing that struck me is much of the code could be simplified through the use of arrays and complementary loops.

For example, these lines:

        button1 = new JButton("");
        button2 = new JButton("");
        button3 = new JButton("");
        button4 = new JButton("");
        button5 = new JButton("");
        button6 = new JButton("");
        button7 = new JButton("");
        button8 = new JButton("");
        button9 = new JButton("");

        panel.add(button1);
        panel.add(button2);
        panel.add(button3);
        panel.add(button4);
        panel.add(button5);
        panel.add(button6);
        panel.add(button7);
        panel.add(button8);
        panel.add(button9);

        button1.addActionListener(this);
        button2.addActionListener(this);
        button3.addActionListener(this);
        button4.addActionListener(this);
        button5.addActionListener(this);
        button6.addActionListener(this);
        button7.addActionListener(this);
        button8.addActionListener(this);
        button9.addActionListener(this);

Could be replaced with:

        JButton[] buttons = new JButton[9];
        for (JButton button : buttons) {
            button = new JButton("");
            panel.add(button);
            button.addActionListener(this);
        }

You can also do your tests for vertical, horizontal in a loop although you'd refer to the index rather than button1 or button2 if you use arrays, so remember that indexing starts at 0.

Extract more logic into methods

You want your tasks to be as modular as possible. Your listener is responsible for:

  1. Processing event triggers.
  2. Altering game state.
  3. Evaluating win conditions.
  4. Displaying game end messages.

This is a heavy workload and if a single thing breaks it may seemingly affect everything else, leading you to try and fix things that don't need fixing, and the compiler to even point at incorrect error locations.

Anytime you see a method or class instantiation and can't get a succinct answer for what it does, it probably does too much. You may not see this now, but if someone else looked at the code or if you came back to it in six months it would be more apparent. Read more about the Single Responsibility Principle.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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