What kind of Wall should it be? (for an RPG)

I'm creating an RPG type game, and now I am working on procedural town generation. The algorithm for that creates some roads going horizontally and vertically, and then attempts to place buildings in the open spaces.

Once a building is successfully placed, I use this algorithm to iterate through the positions of the walls and determine what type of wall should go in each position. There are a few different patterns needed to account for all potential outcomes, such as the intersection between walls, the corners, etc.

Here is a screenshot of the result:

First I created this enum for the different possible patterns.

WallPattern.java

```
public enum WallPattern {

//0 is empty
//1 is wall
//2 is optional, can be empty or wall
TOP_LEFT(0, 0, 2,
0, 1, 1,
2, 1, 2),
TOP_RIGHT(2, 0, 0,
1, 1, 0,
2, 1, 2),
BOTTOM_LEFT(2, 1, 2,
0, 1, 1,
0, 0, 2),
BOTTOM_RIGHT(2, 1, 2,
1, 1, 0,
2, 0, 0),
HORIZONTAL(2, 0, 2,
2, 1, 2,
2, 0, 2),
VERTICAL(2, 2, 2,
0, 1, 0,
2, 2, 2),
INTERSECTION(2, 1, 2,
1, 1, 1,
2, 1, 2);

private final List pattern;

private WallPattern(int one, int two, int three, int four, int five, int six, int seven, int eight, int nine) {
this.pattern = new ArrayList();
this.pattern.add(one);
this.pattern.add(two);
this.pattern.add(three);
this.pattern.add(four);
this.pattern.add(five);
this.pattern.add(six);
this.pattern.add(seven);
this.pattern.add(eight);
this.pattern.add(nine);
}

public boolean patternMatches(List tilePattern) {

if (tilePattern.size() != this.pattern.size()) {
return false;
}

boolean matches = true;
for (int i = 0; i < tilePattern.size(); i++) {
To

Unrelated: you may be interested in this presentation from the folks at Grinding Gear Games, where they discuss random level generation

public enum WallPattern

This might be better named a WallSpecification - it's a description of how to choose what kind of wall piece you need, right? WallPattern isn't wrong, but I think the word "Pattern" can get a bit overloaded.

//0 is empty
//1 is wall
//2 is optional, can be empty or wall

Magic numbers are a bad idea. You can usually identify magic numbers by the fact that you never actually use them as numbers (would you ever add a wall to an optional?), and if you are lucky they are accompanied by a comment explaining that they mean something else.

enum TileSpecification {
   EMPTY, WALL, UNSPECIFIED
}

Introducing TileSpecification is going to make the WallPattern code more verbose, but I believe it will help clarify your intent. Onward....

private final List pattern;

private WallPattern(TileSpecification one, TileSpecification two, TileSpecification three, TileSpecification four, TileSpecification five, TileSpecification six, TileSpecification seven, TileSpecification eight, TileSpecification nine) {
    this.pattern = new ArrayList();
    this.pattern.add(one);
    this.pattern.add(two);
    this.pattern.add(three);
    this.pattern.add(four);
    this.pattern.add(five);
    this.pattern.add(six);
    this.pattern.add(seven);
    this.pattern.add(eight);
    this.pattern.add(nine);
}

Magic numbers again -- this time appearing as argument names. The names of the arguments kind of match up with the index into the list that they will appear in, but that's not telling us what's really going on. Let's look at how it is being used....

for (int i = 0; i < tilePattern.size(); i++) {
        TownTileType type = tilePattern.get(i);
        int patternNum = this.pattern.get(i);

If you look at this code for a moment, we aren't really using the List as a List; we don't care about the order of the checks at all. It's just being used as a Collection, with the index being used as the key to match up the two tile patterns.

The canonical collection that supports lookup by key is a Map

private final Map pattern;

private WallPattern(TileSpecification one, TileSpecification two, TileSpecification three, TileSpecification four, TileSpecification five, TileSpecification six, TileSpecification seven, TileSpecification eight, TileSpecification nine) {
    this.pattern = new HashMap();
    this.pattern.put(1, one);
    this.pattern.put(2, two);
    this.pattern.put(3, three);
    this.pattern.put(4, four);
    this.pattern.put(5, five);
    this.pattern.put(6, six);
    this.pattern.put(7, seven);
    this.pattern.put(8, eight);
    this.pattern.put(9, nine);
}

Here, we are temporarily taking advantage of the fact that Java can autobox int to Integer via Integer.valueOf.

This refactoring added more magic numbers, which isn't what we want. But it is maybe getting easier to see that they are there, and should be addressed. Effective Java tells us that we should use EnumMap instead of ordinal indexing -- a fancy way of saying we shouldn't be using numbers as keys to our map.

Let's look for the meaning behind the numbers

TOP_LEFT(0, 0, 2,
         0, 1, 1,
         2, 1, 2),

Aha - we get a lucky break. You formatted the original code to make it easier to see what was going on, and from this we can determine that meaning encoded into the position in your list is the relative position of this tile in relation to the home tile. Since we can enumerate the relative positions, that suggests we should again implement an enum

enum RelativePosition {
    TOP_LEFT, TOP_CENTER, TOP_RIGHT,
    CENTER_LEFT, HOME, CENTER_RIGHT,
    BOTTOM_LEFT, BOTTOM_CENTER, BOTTOM_RIGHT
} 

private final Map pattern;

private WallPattern(TileSpecification one, TileSpecification two, TileSpecification three, TileSpecification four, TileSpecification five, TileSpecification six, TileSpecification seven, TileSpecification eight, TileSpecification nine) {
    this.pattern = new EnumMap(RelativePosition.class);
    this.pattern.put(TOP_LEFT, one);
    this.pattern.put(TOP_CENTER, two);
    this.pattern.put(TOP_RIGHT, three);
    this.pattern.put(CENTER_LEFT, four);
    this.pattern.put(HOME, five);
    this.pattern.put(CENTER_RIGHT, six);
    this.pattern.put(BOTTOM_LEFT, seven);
    this.pattern.put(BOTTOM_CENTER, eight);
    this.pattern.put(BOTTOM_RIGHT, nine);
}

Now that it is clear what the arguments are, we can name them more appropriately.

private WallPattern(TileSpecification topLeftNeighbor, TileSpecification topCenterNeighbor, TileSpecification topRightNeighbor, TileSpecification centerLeftNeighbor, TileSpecification home, TileSpecification centerRightNeighbor, TileSpecification bottomLeftNeighbor, TileSpecification bottomCenterNeighbor, TileSpecification bottomRightNeighbor) {

Well, I suppose t

StackExchange Code Review Q#106966, answer score: 7