Tic Tac Toe in Swing

Question

This is a Tic Tac Toe game I wrote in Java (Swing). It fully works when detecting the winner or a tie. I'm wondering if there's any way I can improve it or clean it up.

I'm trying to achieve a 3 by 3 simple Tic Tac Toe game. The first player is X and the second player is O.

My X and O buttons are images. When a button is pressed, the value goes up. If it's odd, it's X. If it's even, it's O.

Because I didn't know how to compare images, I stored a parallel String array to compare the values. Should I be using a 2-D array?

/* Tic Tac Toe */

import java.awt.*;
import java.awt.event.*;

import javax.swing.*;

public class TicTacToe implements ActionListener {
    JFrame window = new JFrame("Tic Tac Toe");
    JButton b[] = new JButton[9];

    String letter = "";
    ImageIcon X;
    ImageIcon O;
    ImageIcon ltr;
    int value = 0;
    boolean win = false;
    String[] letters = new String[9];

    public TicTacToe() {
        // Initialize Array
        for (int i = 0; i < 9; i++) {
            letters[i] = "";
        }

        // Assign images
        X = new ImageIcon(getClass().getResource("X.png"));
        O = new ImageIcon(getClass().getResource("O.png"));
        // Create the Window
        window.setSize(500,500);
        window.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        window.setLayout(new GridLayout(3,3));

        // Add Buttons
        for (int i = 0; i < 9; i++) {
            b[i] = new JButton();
            window.add(b[i]);
        }

        // Add ActionListener
        for (int i = 0; i < 9; i++) {
            b[i].addActionListener(this);
        }

        window.setVisible(true);
    }

    public void actionPerformed(ActionEvent a) {
        value++;
        // Who's Turn
        if (value % 2 == 1) {
            ltr = X;
            letter = "X";
        }
        if (value % 2 == 0) {
            ltr = O;
            letter = "O";
        }

        // Display Letters
        for (int i = 0; i < 9; i++) {
            if (a.getSource() == b[i]) {
                b[i].setIcon(ltr);
                b[i].setDisabledIcon(ltr);
                b[i].setEnabled(false);
                letters[i] = letter;
            }
        }

        // Who Won

        // Horizontal
        if (letters[0].equals(letters[1]) && letters[1].equals(letters[2]) && !letters[0].equals("")) {
            win = true;
        } else if (letters[3].equals(letters[4]) && letters[4].equals(letters[5]) && !letters[3].equals("")) {
            win = true;
        } else if (letters[6].equals(letters[7]) && letters[7].equals(letters[8]) && !letters[6].equals("")) {
            win = true;
        }

        // Vertical
        if (letters[0].equals(letters[3]) && letters[3].equals(letters[6]) && !letters[0].equals("")) {
            win = true;
        } else if (letters[1].equals(letters[4]) && letters[4].equals(letters[7]) && !letters[1].equals("")) {
            win = true;
        } else if (letters[2].equals(letters[5]) && letters[5].equals(letters[8]) && !letters[2].equals("")) {
            win = true;
        }

        // Diagonal
        if (letters[0].equals(letters[4]) && letters[4].equals(letters[8]) && !letters[0].equals("")) {
            win = true;
        } else if (letters[2].equals(letters[4]) && letters[4].equals(letters[6]) && !letters[2].equals("")) {
            win = true;
        }

        if (win) {
            JOptionPane.showMessageDialog(null, "Player " + letter + " wins!");
            for (JButton i : b) {
                i.setEnabled(false);
            }
        } else if (!win && value == 9) {
            JOptionPane.showMessageDialog(null, "The game ended in a tie.");
        }
    }

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

Answer 1

1. Change your fields' visibility to private as others object shouldn't know (nor care) about the inner working of your TicTacToe, this is known as Encapsulation
2. Turn the buttons into a 2D array and make the name clearer (b isn't really informative ^^)
3. Avoid magic numbers (talking about 3, 9, 500 which can be replaced by GRID_LENGTH, GRID_SIZE...) : this way, not only it's clearer for a newcomer but if you ever change one value it'll be updated everywhere
4. I don't think there is any point in making win a field, it's only used like a local variable
5. Turn the // WHO WON part into a method that'll do all the checks for you and return you a boolean : in general, calling multiple short methods makes for a clearer code as the code is more declarative this way (and it removes useless comments)
6. For same reasons as above, turn the victory message printing into its own method
7. Use a for loop to check every lines and every columns without caring for their size, that'll make for a clearer code and it'll also be easier to modify the current behaviour (if you ever want to allow for differents grid size)
8. You may want to not compute diagonals unless someone has checked one of the diagonal value, it'll probably slighty increase cyclomatic complexity though so it's up to you ;)
9. You forgot to disable the buttons if the game ends in a tie... in the same way as points 5 and 6, creating a disableTicTacToe or disableFrame method may be neat
10. Maybe you should consider offering the players a new game when one has ended

Answer 2

MVC

Because I didn't know how to compare images, I stored a parallel String array to compare the values. Should I be using a 2-D array?

You have to separate concerns and responsibilities.

Any application consists of 3 layers:

1. The Model which represents the applications state. This is the bottom layer. It does not have business logic. Only some infrastructure to notify about changes of data who ever wants to know it...

2. The controller which provides logic working on the model, changing the applications state. This is the medium layer and obviously knows the model. It does not know (and therefore not change) the view.

3. The view which displays the applications state and handles user input delegating it to the controller. It knows the model and display data from it. It also knows the controller and uses it to change data in the model.

In your approach the images are part of the View. They are only used to visualize the actual owner. Your letters array is your model which could be a 2D array as well as you assumed or any other data structure representing the state of fields.

The controller layer should be the ActionListener. Unfortunately you let your main class implement action listener which prohibits separate specialized implementations for each button which would better fit to an OO approach and make the code better to read and better to understand.

OOP

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• single responsibility
• separation of concerns
• KISS (Keep it simple (and) stupid.)
• DRY (Don't repeat yourself.)
• "Tell! Don't ask."
• Law of Demeter ("Don't talk to strangers!")

single responsibility - separation of concerns

separation comments

Your code fails to follow SRP/SoC. This is indicated by the comments you place in your methods to "structure" them. You should move that separated parts to (private) methods of their own with names derived from the comments you put.

mighy methods

Your actionPerformed() method does too much.

• it actually changes the model
• it calculates the game state
• it displays the games end state

This three parts should also live in methods of their own.

DRY (Don't repeat yourself.)

In your action listener method you do the same thing multiple times only changing the indices. This code should be generalized and moved into parametrized methods.

KISS (Keep it simple (and) stupid.)

There is no need to store the buttons in an array of their one (when having a model). The Buttons position at the GUI does not matter anymore after its been connected to a position in the model which it displays and manipulates (through the controller). The doubled feature may lead to inconsistencies when your code evolves.