Slide-Game in Swing

Question

This was done as an assignment in school. As I am still learning some of the basic parts of Java be merciless when it comes to my code. As it is an assignment, some things are unchangeable. The specification requires:

• Swing. So no third-party library is allowed.
• Cell class with getCol() and getRow() methods.
• For the final product to be a Slide-Game.

Some extra features that I tried to implement in a good way.

• Dyanmic handle for rows and cols. It should work for anything over 2 rows and 2 cols.
• Victory check and click tracker.

GUI.java

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

import java.util.ArrayList;
import java.util.List;

public class GUI implements ActionListener
{
    public static final int WINDOW_MIN_X = 240;
    public static final int WINDOW_MIN_Y = 200;

    public static final int COLS = 4;
    public static final int ROWS = 4;
    public static final int HGAP = 3;
    public static final int VGAP = 3;

    public static final int NUMBER_OF_CELLS = ROWS * COLS;

    private JFrame frame;
    private List<Cell> buttons = new ArrayList<>();
    private List<Cell> correct = new ArrayList<>();
    private Cell emptyButton;

    private int movesCounter;

    /**
     * Constructor for objects of class GUI
     */
    public GUI ()
    {
        frame = new JFrame("Slide Game");

        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);

        frame.setMinimumSize(new Dimension (WINDOW_MIN_X, WINDOW_MIN_Y));

        frame.setLayout (new GridLayout (ROWS, COLS, HGAP, VGAP));

        for( Integer i = NUMBER_OF_CELLS-1; i >= 0 ; i-- ) {
            createCell (i);
        }

        emptyButton = buttons.get (NUMBER_OF_CELLS-1);

        frame.setVisible(true);
        frame.pack();

        setVictoryCondition ();
    }

    public void actionPerformed (ActionEvent e) {
        if (checkNeighbour((Cell)e.getSource())) {
            swap ((Cell)e.getSource());
            movesCounter++;
            checkVictory ();
        } else {
            Toolkit.getDefaultToolkit().beep();
        }
    }

    /**
     * createCell (String s)
     * 
     * Creates a Cell and adds an ActionListener
     * 
     * @param   String s
     */
    private void createCell (int v) {
        Cell c = new Cell (v);
        c.addActionListener(
            (ActionEvent e) -> {actionPerformed(e);}
        );
        buttons.add(c);
        frame.add(c);
    }

    /**
     * checkNeighbour (Cell c)
     * 
     * Checks if c is within 1 grid of emptyButton (non-diagonal)
     * 
     * @param   Cell c
     * 
     * @return false if c is more than one grid away from emptyButton
     */
    private boolean checkNeighbour (Cell c) {
        int x = Math.abs(c.getCol() - emptyButton.getCol());
        int y = Math.abs(c.getRow() - emptyButton.getRow());

        if (!(x == 0 && y == 1 || y == 0 && x == 1)) {
            return false;
        }

        return true;
    }

    /**
     * swap (Cell c)
     * 
     * Swaps c with emptyButton
     * 
     * @param   Cell c
     */
    private void swap (Cell c) {
        // swap c and empty
        String tmp = emptyButton.getText ();
        emptyButton.setText (c.getText ());
        c.setText (tmp);

        // update reference to empty button
        emptyButton = c;
    }

    /**
     * setVictoryCondition ()
     * 
     * Initiates correct list
     */
    private void setVictoryCondition () {
        for (Integer i = 1; i < NUMBER_OF_CELLS; i++) {
            correct.add (new Cell (i));
        }
        correct.add(new Cell (0));
    }

    /**
     * checkVictory ()
     * 
     * Compares buttons to correct lists, if match declare victory!
     */
    private void checkVictory () {        
        for (int i = 0; i < NUMBER_OF_CELLS; i++) {
            if (!(buttons.get(i).getText ().equals( correct.get(i).getText ()))) {
                return;
            }
        }

        JOptionPane.showMessageDialog(null, "You achieved victory in " + movesCounter + " moves.",
            "Congratulations!", JOptionPane.INFORMATION_MESSAGE);
    }
}

Cell.java

import javax.swing.*;

public class Cell extends JButton
{
    private static final long serialVersionUID = 8822510600483479089L;
    private int row;
    private int col;

    public Cell (Integer value)
    {
        if (value == 0) {
            setText (" "); 
        } else {
            setText(value.toString());
        }

        row = GUI.NUMBER_OF_CELLS / GUI.COLS-1 - ( value / GUI.COLS);
        col = GUI.NUMBER_OF_CELLS-1 - (row*GUI.COLS + value);

    }

    public int  getCol (){
        return col;
    }

    public int getRow (){
        return row;
    }
}

In short, the code is working. I am fairly happy with the result. I wonder if (and if so where) there are room for improvements.

Answer 1

Cell class

public class Cell extends JButton

I would not extend JButton. It is better to use composition/encapsulation and store a JButton inside the Cell class. Right now you can call plenty of JButton methods from outside the Cell class that is totally irrelevant to the Cell itself. Just check the many inherited methods that a JButton has!

---

if (!(buttons.get(i).getText ().equals( correct.get(i).getText ())))

No need to wrap it in an extra set of parenthesis, and please watch your spacing.

if (!buttons.get(i).getText().equals(correct.get(i).getText()))

However, when you apply the advice above and encapsulate the JButton inside the Cell class, you can use something like this in your Cell class:

int getNumber() {
    return this.number;
}

void setNumber(int number) {
    this.number = number;
    this.button.setText(number == 0 ? " " : String.valueOf(number));
}

If you are unsure about that a ? b : c syntax. You can also use an if-else statement:

    if (number == 0) {
        this.button.setText(" ");
    } else {
        this.button.setText(String.valueOf(number));
    }

---

public Cell (Integer value)

You are using Integer instead of int in your entire program. You can use a primitive int instead, which is preferred.

---

row = GUI.NUMBER_OF_CELLS / GUI.COLS-1 - ( value / GUI.COLS);
col = GUI.NUMBER_OF_CELLS-1 - (row*GUI.COLS + value);

You are making your Cell dependent on your GUI here. This is generally a bad idea. I would instead pass the row and col to the Cell as constructor arguments.

GUI class

if (!(x == 0 && y == 1 || y == 0 && x == 1)) {
    return false;
}

"If not x equals zero and y equals one or y equals 0 and x equals one." Please don't have me read that again.

I would add extra parenthesis around x == 0 && y == 1 to avoid mixing up && and ||.

Also, by switching the logic around it becomes this, which is clearer:

if ((x == 0 && y == 1) || (y == 0 && x == 1)) {
    return true;
}
return false;

This can be rewritten to:

return (x == 0 && y == 1) || (y == 0 && x == 1);

Which is the same as:

return x + y == 1;

Ain't some math beautiful? :)

---

c.addActionListener(
    (ActionEvent e) -> {actionPerformed(e);}
);

+1 for using Lambdas! You can write this as:

c.addActionListener(this::actionPerformed);

Answer 2

You're off to a good start! The code is nicely written, easily readable, simple and it works. Here are a couple of tips though, on top of @Simon's excellent review.

Victory condition

It's odd that you're using a second list of cells to check the victory condition. It's especially strange that you don't reuse the original cells, but create a fresh new set of cells, and perform the check by comparing the output of getText. This last point is especially bad, because button labels are not designed to be logical elements of the program, only visualization (hence their type, String).

A better way would be to give the Cell class an explicit value field, and check the victory condition using that, instead of the extra set of buttons and the getText method, for example:

private void checkVictory() {
    for (int i = 0; i < NUMBER_OF_CELLS - 1; i++) {
        if (buttons.get(i).getValue() != i + 1) {
            return;
        }
    }

This also means that when swapping a cell with the empty cell, you also need to swap the values:

    int value = c.getValue();
    c.setValue(emptyButton.getValue());
    emptyButton.setValue(value);

It's a bit of extra work, but I think it's worth it.

Naming

Instead of checkNeighbour, a more common way to name a method that returns a boolean is to replace "check" with "is". This will intuitively distinguish it from other methods that check something without returning a value, such as checkVictory.

Other minor improvements

Make final member variables as much as possible. For example these in GUI:

private final JFrame frame;
private final List<Cell> buttons = new ArrayList<>();
private final List<Cell> correct = new ArrayList<>();

And these in Cell:

private final int row;
private final int col;