How can I clean up this method I wrote in Java?

Question

I'm working on writing a Maze generator. I have a "Cell" class which is as follows:

public class Cell {
    public boolean northWall;
    public boolean southWall;
    public boolean eastWall;
    public boolean westWall;

    public Cell north;
    public Cell south;
    public Cell east;
    public Cell west;

    public boolean visited;

    public Cell() {
     northWall = true;
     southWall = true;
     eastWall = true;
     westWall = true;
     visited = false;
    }

    public boolean hasUnvisitedNeighbors() {
     return ((north != null && !north.Visited)
       || (south != null && !south.Visited)
       || (east != null && !east.Visited) || (west != null && !west.Visited));
    }

    public Cell removeRandomWall() {
     List<Cell> unvisitedNeighbors = new ArrayList<Cell>();
     if (north != null && !north.Visited)
      unvisitedNeighbors.add(north);
     if (south != null && !south.Visited)
      unvisitedNeighbors.add(south);
     if (west != null && !west.Visited)
      unvisitedNeighbors.add(west);
     if (east != null && !east.Visited)
      unvisitedNeighbors.add(east);



     if (unvisitedNeighbors.size() == 0) {
      return null;
     } else {
      Random randGen = new Random();
      Cell neighbor = unvisitedNeighbors.get(randGen
        .nextInt((unvisitedNeighbors.size())));

      if (neighbor == north) {
       northWall = false;
       north.southWall = false;
       return north;
      } else if (neighbor == south) {
       southWall = false;
       south.northWall = false;
       return south;
      } else if (neighbor == west) {
       westWall = false;
       west.eastWall = false;
       return west;
      } else if (neighbor == east) {
       eastWall = false;
       east.westWall = false;
       return east;
      }

      return null;
     }

    }
}

A maze in my program is simply a 2d dimensional array of Cells. After I create the array I manually go in and set all the references to the adjancent cells (North, South, East, West).

What I'm trying to clean up is removeRandomWall(). It is suppose to randomly choose an adjacent Cell which has its visited flag set to false, and remove the wall in both this cell and the adjacent cell that connects them.

So, it needs to consider all adjacent cells who haven't been visited, randomly choose one, then set the wall in this cell and the adjacent one to false so there is a path between them now. I've attempted it above but it seems very cludgey.

Can anyone help me out?

Answer 1

Instead of having 4 separate members:

public Cell North;
public Cell South;
public Cell East;
public Cell West;

just have 1 array of them:

public Cell [] cells = new Cell[4];

And 4 constants:

public final int NORTH = 0;
public final int EAST = 1;
public final int SOUTH = 2;
public final int WEST = 3;

it makes things like remove random wall so much easier.

Answer 2

Make an enumeration for the direction, then access the walls and the neighbors by giving the direction.

You should make your member variables private and write them in lower case.

Answer 3

A first attempt, I would make the member variable private final if you can:

public class Cell {
    public boolean NorthWall;
    public boolean SouthWall;
    public boolean EastWall;
    public boolean WestWall;

    public Cell North;
    public Cell South;
    public Cell East;
    public Cell West;

    public boolean Visited;

    public Cell() {
        NorthWall = true;
        SouthWall = true;
        EastWall = true;
        WestWall = true;
        Visited = false;
    }

    public boolean hasUnvisitedNeighbors() {
        return unvisited(North) || unvisited(South) || unvisited(East) || unvisited(West);
    }

    private List<Cell> getUnvisitedNeighbors() {
        List<Cell> result = new ArrayList<Cell>();
        if (unvisited(North))
            result.add(North);
        if (unvisited(South))
            result.add(South);
        if (unvisited(West))
            result.add(West);
        if (unvisited(East))
            result.add(East);
        return result;
    }

    private boolean unvisited(Cell cell) {
        return cell != null && !cell.Visited;
    }

    private Cell getRandomUnvisitedNeighbor() {
        Random randGen = new Random();
        List<Cell> unvisitedNeighbors = getUnvisitedNeighbors();
        return unvisitedNeighbors.get(randGen.nextInt((unvisitedNeighbors.size())));
    }

    public Cell removeRandomWall() {
        if (!hasUnvisitedNeighbors()) {
            return null;
        }
        Cell neighbor = getRandomUnvisitedNeighbor();
        if (neighbor == North) {
            NorthWall = false;
            North.SouthWall = false;
        } else if (neighbor == South) {
            SouthWall = false;
            South.NorthWall = false;
        } else if (neighbor == West) {
            WestWall = false;
            West.EastWall = false;
        } else if (neighbor == East) {
            EastWall = false;
            East.WestWall = false;
        }

        return neighbor;

    }
}

Answer 4

Try to remove checks for null, by using a "null Cell" object...

public static final Cell NULL_CELL = new Cell();

so where you would have use null to indicate that no such cell existed, you can now use NULL_CELL.

Now you can replace

if (north != null && !north.Visited)
                unvisitedNeighbors.add(north);

with

if (!north.isVisited()) {
    unvisitedNeighbors.add(north);
}

Typically in Java, member variables are private, and you use "getters" to access them...

private boolean northWall;
private boolean southWall;
private boolean eastWall;
private boolean westWall;

private Cell north;
private Cell south;
private Cell east;
private Cell west;

private boolean visited;

public boolean hasNorthWall() {
  return northWall;
}

public Cell getNorthCell() {
  return north;
}

// etc.