MazeRunner in Java

To test the queue I perfected from one of my CR questions, I've developed a maze program that receives an input maze, starting and end positions, and solves it. I have separate software that handles the maze execution and file parsing. I'm curious as to whether my implementation is optimal (fully expecting it to not be), and was wondering what could be done to optimize it.

Just for reference: I renamed my class from DoublingQueue to ArrayQueue.

Maze.java

```
public class Maze {

public final static char WALL = 'X', PATH = '.';

private static char[][] maze;

private Location start;
private Location end;

public Maze(char[][] puzzle, Location startPos, Location endPos) {
maze = puzzle;
start = startPos;
end = endPos;
}

public int size() {
return maze.length;
}

public void print() {
for (int i = 0; i = 0 && i = 0 && j candidates = new ArrayQueue();

Location current;
Location next;

candidates.enqueue(new Location(start.i(), start.j()));

while (!candidates.isEmpty()) {
current = candidates.dequeue();

if (isFinal(current)) {
break;
}

mark(current, PATH);

next = current.north();

if (isInMaze(next) && isClear(next)) {
candidates.enqueue(next);
}

next = current.east();

if (isInMaze(next) && isClear(next)) {
candidates.enqueue(next);
}

next = current.west();

if (isInMaze(next) && isClear(next)) {
candidates.enqueue(next);
}

next = current.south();

if (isInMaze(next) && isClear(next)) {
candidates.enqueue(next);
}
}

if (candidates.isEmpty()) {
System.out.println("You're stuck in the maze!");
} else {
System.out.println("You got it!"

All your Maze belong to Maze.maze

The cells of the maze are in a static char[][]. Multiple Maze instances will share this, which will be unexpected and surely lead to bugs. This is especially troubling considering that the constructor takes a char[][] parameter, replacing the static data. So if I create a Maze with some data cells1, and another Maze with some other data cells2, both instances will use cells2.

Single responsibility principle

The solve method does too much:

• find path from start to end

• reset the maze

• print the completed maze

• print if path could be found

It would be better if solve did just that: solve the maze. All the other steps should be outside of this method.

Naming

maze is a poor name for cells. So is the constructor parameter puzzle.

It's confusing to use similar names for different purposes, such as Maze for the class and maze for the char[][] field.

It's confusing to use different names for the same purpose, such as maze and puzzle for the char[][] data.

With the names improved, the constructor could be:

public Maze(char[][] cells, Location start, Location end) {
    this.cells = cells;
    this.start = start;
    this.end = end;
}

i and j are very poor names for location coordinates in Location.
These names are most commonly used as loop variables, and rarely appropriate for other purposes. row and col would be better.

What's the size of a maze?

The implementation assumes that the width and height are the same.
You could add an assert in the constructor to document that fact:

public Maze(char[][] cells, Location start, Location end) {
    assert cells.length == cells[0].length : "the width and height of the maze should be the same";

    this.cells = cells;
    this.start = start;
    this.end = end;
}

A better solution would be to change the implementation to mazes where the width and height can be different. It looks easy to do.

Unused methods

The isMarked methods are unused. They were strange anyway, considering that the mark methods take a char parameter and isMarked don't, which begs the question: is marked what? In any case, as these methods are unused, they should be removed.

Unused return values

The return value of the mark method is never used. As such, this method should be void, and then it can be simplified.

Inappropriate clone

The clone() method is special in Java. It should be used only to override Object's clone() method to avoid confusion. (However, in general, it should not be used at all. See Effective Java by Joshua Bloch for a detailed explanation.)

A propose the name copyCells instead. I added the "Cells" suffix, because that's the only thing that is copied, Mace.clone and Maze.copy would suggest a copy of the entire maze, returning a Maze instead of a char[][].

And the implementation can be simplified, by using the clone() method of arrays:

public char[][] copyCells() {
    char[][] copy = cells.clone();

    for (int i = 0; i < size(); i++) {
        copy[i] = cells[i].clone();
    }

    return copy;
}

Incorrect use of generics

The type parameter is missing at the right hand size:

ArrayQueue candidates = new ArrayQueue();

Add <> to correct it:

ArrayQueue candidates = new ArrayQueue<>();

Duplicated code

As you check each direction, you have this condition repeated 4 times:

if (isInMaze(next) && isClear(next)) {

It would be better to move this condition to a helper method, so that you can reduce the duplicated logic, for example:

if (isPossibleMove(next)) {

Make final what can be final

The fields maze, start and end are never reassigned, so it's good to make them final.

Modifier ordering

It's a minor thing, but instead of:

public final static char WALL = 'X', PATH = '.';

The recommended ordering of modifiers is this:

public static final char WALL = 'X', PATH = '.';

StackExchange Code Review Q#123349, answer score: 3