programmed 15 puzzle in java

Question

This is my code for a 15 puzzle.

The basic gist is to get a grid of equal width and height (size) and number them in ascending order, with the exception of the last square getting a 0 value ('0' integer not shown on the tile so it looks like a blank tile). The grid is scrambled and then the user has to click tiles either next to or above the 'blank tile' to swap their places. The grid goes green when it is reordered in ascending order with the 'blank tile' being at last spot on the grid.

The default size is 4, hence the 15 puzzle name, but my 15 puzzle accepts any size (within reason).

FifteenPuzzle

import java.util.Arrays;
public class FifteenPuzzle
{ 
    private int[][] grid; 
    private int xspace;   // xspace,yspace are the current coordinates of the space
    private int yspace;
    private final int size; // the number of tiles across and down 
    private final int[][] goal; // the tile positions in the goal state
    private int movecount = 1000;

    //sets up the grid using initialGrid

    public FifteenPuzzle (int[][] initialGrid)
    {
        size = initialGrid.length;
        goal = setGoal();
        grid = initialGrid;

        for (int x = 0; x < size; x++) {
            for (int y = 0; y < size; y++) {
                if (grid[x][y] == 0) {
                    xspace = x;
                    yspace = y;
                }
            }
        }

    }

    // sets up the grid by copying goal and then making random moves. 

    public FifteenPuzzle (int size)
    {

        if (size < 2 || size > 10) {
            throw new IllegalArgumentException("Invalid size");
        }
        this.size = size;
        goal = setGoal();

        grid = new int[size][size];
        for (int x = 0; x < size; x++) {
            for (int y = 0; y < size; y++) {
                grid[x][y] = goal[x][y];
                if(goal[x][y] == 0) {
                    xspace = x;
                    yspace = y;
                }
            }
        }
        scramble();
    }

    public void scramble() {

        for(int moves = 1; moves <= movecount; moves++){
            //do a set amount of random moves.

            boolean done = false;
            while (!done) {
                double num = Math.random();

                if (num <= 0.25) {
                    if(legalClick(xspace + 1, yspace)) {
                        moveTile(xspace + 1, yspace);
                        done = true;
                    }
                }
                if (num > 0.25 && num <= 0.5) {
                    if(legalClick(xspace - 1, yspace)) {
                        moveTile(xspace - 1, yspace);
                        done = true;
                    }
                }
                if (num > 0.5 && num <= 0.75) {
                    if(legalClick(xspace, yspace + 1)) {
                        moveTile(xspace, yspace + 1);
                        done = true;
                    }
                }
                if (num > 0.75) {
                    if(legalClick(xspace, yspace - 1)) {
                        moveTile(xspace, yspace - 1);
                        done = true;
                    }
                }
            }
        }
    }

    public int[][] setGoal() {
        int[][] goal = new int[size][size];
        for (int a = 1; a <= size; a++) {
            for (int b = 1; b <= size; b++) {
                goal[a-1][b-1] = a + size*(b-1);
            }
        }
        goal[size-1][size-1] = 0;
        return goal;
    }


    public int[][] getGrid()
    {
        return grid;
    }

    public int getSize()
    {
        return size;
    }


     // Returns true if x,y is on the board and adjacent to the blank space. 

    public boolean legalClick(int x, int y)
    {

        if (x < 0 || y < 0 || x > size-1 || y > size-1) {
            return false;
        }

        return ( (x == xspace && (y == yspace - 1 || y == yspace + 1)) || (y == yspace && (x == xspace - 1 || x == xspace + 1)) );
    }

    public boolean finished()
    {

        return Arrays.deepEquals(grid, goal);
    }

   // swaps the tile at x,y with the space;

    public void moveTile (int x, int y) 
    {

        if (!legalClick(x, y)) {
            return;
        }
        grid[xspace][yspace] = grid[x][y];
        xspace = x;
        yspace = y;
        grid[xspace][yspace] = 0;
    }

}

SimpleCanvas

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

public class SimpleCanvas {
    private JFrame frame;
    private CanvasPane canvas;
    private Graphics2D graphic;
    private Image canvasImage;
    private boolean autoRepaint;


    public SimpleCanvas(String title, int width, int height, Color bgColour) {
        frame = new JFrame();
        canvas = new CanvasPane();
        frame.setContentPane(canvas);
        frame.setTitle(title);
        canvas.setPreferredSize(new Dimension(width, height));
        frame.pack();
        Dimension size = canvas.getSize();
        canvasImage = canvas.createImage(size.width, size.height);
        graphic = (Graphics2D) canvasImage.getGraphics();
        graphic.setColor(bgColour);
        graphic.fillRect(0, 0, size.width, size.height);
        graphic.setColor(Color.black);
        frame.setVisible(true);
        this.autoRepaint = true;

    }


    public void drawLine(int x1, int y1, int x2, int y2, Color c) {
        setForegroundColour(c);
        graphic.drawLine(x1, y1, x2, y2);

    }

    public void drawRectangle(int x1, int y1, int x2, int y2, Color c) {
        setForegroundColour(c);
        graphic.fillRect(Math.min(x1, x2), Math.min(y1, y2), Math.abs(x1 - x2), Math.abs(y1 - y2));

    }

    public void drawString(String text, int x, int y, Color c) {
        setForegroundColour(c);
        graphic.drawString(text, x, y);
        graphic.setFont(new Font("TimesRoman", Font.BOLD, 20)); 
        if (autoRepaint)
            canvas.repaint();
    }


    public void drawString(int n, int x, int y, Color c) {
        drawString(n + "", x, y, c);
    }

    public void setForegroundColour(Color newColour) {
        graphic.setColor(newColour);
    }

    public Color getForegroundColour() {
        return graphic.getColor();
    }

    public void setAutoRepaint(boolean autoRepaint) {
        this.autoRepaint = autoRepaint;
    }


    public void repaint() {
        canvas.repaint();
    }


    public void addMouseListener(MouseListener ml) {
        canvas.addMouseListener(ml);
    }

    public void addMouseMotionListener(MouseMotionListener mml) {
        canvas.addMouseMotionListener(mml);
    }

    class CanvasPane extends JPanel {
        public void paint(Graphics g) {
            g.drawImage(canvasImage, 0, 0, null);
        }
    }
}

FifteenPuzzleViewer

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

public class FifteenPuzzleViewer implements MouseListener
{
    private FifteenPuzzle puzzle; 
    private int size;   
    private SimpleCanvas sc;     


    public FifteenPuzzleViewer(FifteenPuzzle puzzle) 
    {

        this.puzzle = puzzle;
        size = puzzle.getSize();
        sc = new SimpleCanvas(size*size-1 + " Puzzle", 100*size, 100*size + 100, Color.white);
        sc.addMouseListener(this);
        if(puzzle.finished()) {
            drawGreenGrid();
        } else {
            drawGrid();
            drawGrid();
        }

    }

    private void drawGrid()
    {

        int[][] grid = puzzle.getGrid();
        for (int y = 0; y < size; y++) {
            for (int x = 0; x < size; x++) {
                drawTile(x*100, y*100, Color.RED);
                if(grid[x][y] != 0 && grid[x][y] < 10) {
                    sc.drawString(grid[x][y], x*100+45, y*100+55, Color.BLACK);
                } else if (grid[x][y] != 0) {
                    sc.drawString(grid[x][y], x*100+40, y*100+55, Color.BLACK);
                }
            }
        }

        for (int y = 0; y < size + 1; y++) {
            sc.drawLine(0,y*100,size*100,y*100, Color.BLACK);
        }

        for (int x = 0; x < size; x++) {
            sc.drawLine(x*100,0,x*100,size*100, Color.BLACK);
        }
    }

    private void drawGreenGrid()
    {

        int[][] grid = puzzle.getGrid();
        for (int y = 0; y < size; y++) {
            for (int x = 0; x < size; x++) {
                drawTile(x*100, y*100, Color.GREEN);
                if(grid[x][y] != 0 && grid[x][y] < 10) {
                    sc.drawString(grid[x][y], x*100+45, y*100+55, Color.BLACK);
                } else if (grid[x][y] != 0) {
                    sc.drawString(grid[x][y], x*100+40, y*100+55, Color.BLACK);
                }
            }
        }

        for (int y = 0; y < size + 1; y++) {
            sc.drawLine(0,y*100,size*100,y*100, Color.BLACK);
        }

        for (int x = 0; x < size + 1; x++) {
            sc.drawLine(x*100,0,x*100,size*100, Color.BLACK);
        }
    }

    private void drawTile(int x, int y, Color c)
    {

        sc.drawRectangle(x, y, x+100, y+100, c);
    }

    public void mousePressed(MouseEvent e)  
    {

        puzzle.moveTile(e.getX()/100, e.getY()/100);

        if(puzzle.finished()) {
            drawGreenGrid();
        } else {
            drawGrid();
        }

    }

    public void mouseReleased(MouseEvent e) {}
    public void mouseExited(MouseEvent e)   {}
    public void mouseClicked(MouseEvent e)  {}
    public void mouseEntered(MouseEvent e)  {}

    public static void main(String[] args) {
        FifteenPuzzle p = new FifteenPuzzle(3);
        FifteenPuzzleViewer fpv = new FifteenPuzzleViewer(p);
    }
}

Answer 1

Design

I'd create a separate class for Grid and Position at the very least. Especially when separating out the Grid the game code will be significantly reduced. Smaller classes should generally be preferred when they make sense.

With the removal of movecount (see below), you'd have just two fields: currentGrid and goalGrid, both of type Grid.

Code review

I'll handle the lines in order, skipping many, with the remarks below the code.

import java.util.Arrays;
public class FifteenPuzzle
{ 

Please use at least a white line between import statements and the class declaration.

The class should probably be made final unless you create it for extension.

Using { on a new line instead of "Egyptian braces" is often acceptable, but it is not very much used for Java. If they are used, then use them consistently (which is not the case here). Most IDE's contain code formatting nowadays, use that if you don't like doing it yourself.

private int xspace;   // xspace,yspace are the current coordinates of the space
private int yspace;

What space? The empty space? Please be precise. If two variables are needed for one thing, try and aggregate them together, e.g. into a Position class (immutable with equals and hashCode please).

private final int size; // the number of tiles across and down 

Not required, grid.length will do that. Generally, try to minimize the amount of fields. However, for readability reasons I guess this is kinda OK.

private int movecount = 1000;

The field movecount (or rather moveCount) should be a constant; it doesn't change within the game logic.

I'm a bit worried about the naming though, what about, say, edgeSize or dimension?

grid = initialGrid;

Why isn't the size of the grid checked here? It is not symmetric with the constructor that just takes the grid size. Furthermore, there is no check that the width and height are identical.

public void scramble() {

Hey, is the user allowed to call scramble? If not, then why is the method public instead of private? Same for other methods.

done = true;

Use while (true) or for(;;) and break / continue instead. May look weird initially, but it removes one variable from the equation, and you can remove redundant checks.

for (int a = 1; a <= size; a++) {

No, no, no. Always use zero based indexing:

for (int a = 0; a < size; a++) {

unless there are specific reasons not to. It seems OK for the move counter (as you're doing move 1 initially, not move 0 I suppose).

And what about that naming, why a and b for coordinates? x and y is used correctly in the rest of the code.

public int[][] setGoal() {

A "setter" is a specific function that sets a field to a given value. It takes a single argument and returns void. createGoal is probably a better name.

public int[][] getGrid()

This is a correct "getter" declaration. No arguments, single return of a field.

return grid;

Never ever return a mutable reference to something that makes up the internal state of a class, violating encapsulation principles. Copy or clone the data instead, or provide read only access (iterator, lambda).

if (x < 0 || y < 0 || x > size-1 || y > size-1) {

This I can read somewhat.

return ( (x == xspace && (y == yspace - 1 || y == yspace + 1)) || (y == yspace && (x == xspace - 1 || x == xspace + 1)) );

This I cannot. The line size is too large as well. Compare to reading this:

return insideGrid(x, y) && nextToEmpty(x, y);

and then implementing those functions. With a bit of help of the IDE that's not even that much work.

if (!legalClick(x, y)) {
    return;
}

Just returning (or worse, just returning null or 0 for methods that return data) is almost always a bad decision. In this case an exception should be thrown.

If the calling code first wants to check if the move is valid, it can call legalClick itself. You may be calling legalClick twice that way, but the JIT compiler will probably notice if it starts to take too much time.

GUI code

There was some time required to get the GUI running. However, when I did the game ran, but there were some issues. For instance, you can scale the window containing puzzle, but that just mucks up the contents. The puzzle is not scaled well initially either, and a line seems to be missing on the right hand side.

I don't expect to interact with a Viewer. Maybe with a View, but a Viewer sounds rather passive.

There are dual calls to drawGrid that aren't explained at all. I don't see why those are necessary.

I presume the squares are 100 pixels in size or so, but there is a lot of repetition of the value 100. The value 100 should probably be a constant, but generally I'd already use a squareEdgeSize variable for that. At least you did make sure that the value is not repeated in the canvas code, so that's OK.

You are using a MouseListener with unimplemented methods. However, in that case you might want to have a look at a MouseAdapter instead that provides default implementations, cleaning up your listener interface.

When you look at the drawRectangle you can see that you specifically have to calculate x + 100, while the original method of drawing in Java uses a width. So you are calculating a second x using the width only to reverse that later.

Your methods are not symmetric. You have a drawGrid() that doesn't take any arguments, and many other draw methods that do. That kind of inconsistencies are also seen in the whitespace and such. It makes the code less clean and harder to read.

Both the constructor of the viewer and another method draw the grid, and make the same decision which one to draw. This goes against DRY principles.