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.

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.

Community · Accepted Answer · 2020-06-10 13:24:26Z

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);

public class Cell extends JButton was in the specification sorry, should have made that more clear. However I now know for the future. I do know about a ? b : c and tried to make use of it. I was using an Integer to have direct access to Integer.toString(). Passing arguments to Cell - Check. Fair enough. return x + y == 1; that just looks like magic to me. — Emz
– Emz, Commented Jan 24, 2015 at 18:50
@Emz Tell the specification that it is a bad idea like that! By using String.valueOf() you don't need to use Integer.toString(). return x + y == 1 is mathematics, not magic ;) It will return true whenever the sum of x and y equals to 1, which is essentially what you are checking for. — Simon Forsberg
– Simon Forsberg, Commented Jan 24, 2015 at 19:13
@SimonAndréForsberg return x + y == 1; not the same as return (x == 0 && y == 1) || (y == 0 && x == 1);. In the first case x = -4, y = 5 will yield true, in the second case false. — 11684
– 11684, Commented Jan 24, 2015 at 20:00

janos · Accepted Answer · 2015-01-24 18:52:52Z

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;

janos, forgot about the final keyword. Just inexperience from my side. I did that way in victoryCheck if someone wanted to add a specific pattern instead of having the simple 1234,5678... — Emz
– Emz, Commented Jan 24, 2015 at 21:28
You can still use specific patterns, by putting the numbers in a list, and compare buttons.get(i).getValue() with correct.get(i) instead of i + 1 as in the example I gave you — janos
– janos, Commented Jan 24, 2015 at 21:40

Stack Exchange Network

Slide-Game in Swing

2 Answers 2

Cell class

GUI class

Victory condition

Naming

Other minor improvements

You must log in to answer this question.

Linked

Hot Network Questions

Slide-Game in Swing

2 Answers 2

Cell class

GUI class

Victory condition

Naming

Other minor improvements

You must log in to answer this question.

Linked

Related

Hot Network Questions