6
\$\begingroup\$

I am writing a ASCII dungeon creator inspired from games such as Angband, Moria etc.

The main goal of the project is to allow a user to come along and implement their own "DungeonLevelGenerator" so they can generate dungeons however they wish using the interface and tools I've provided to help speed it up.

Let me just explain the source briefly. Everything in the net.woopa.dungeon.core package is implementation of the API. Everything not in the package is my proposed API.

My questions are:

  1. Firstly I used a Visitor Pattern for the Materials, allowing users to create their own materials using an enum that can be used within their project with ease. Is this intuitive for the programmer?

  2. Following this is the API easy to pickup even with little documentation?

  3. Is the performance reduced in any way by writing it as shown.

  4. I have a blend of object and static classes, would it be best for the user to have them all static? (Or alternatively all objects)?

  5. I always try good programming practices (for instance not including hard coded strings and ints) now there are some in there but I plan on moving them soon. Do you feel that this code achieves this with the enums for Materials, Schematics etc?

  6. Moving to the implementation (net.woopa.dungeon.core) is this as efficient as it could be, I feel like it can be difficult to read in places.

CODE: Material.java

    package net.woopa.dungeon.datatypes;
    public interface Material {
    public char getSymbol();
    public String getName();
    public Boolean isChest();
    public Boolean isWall();
    public Boolean isFloor();
    public Boolean isBoundary();
    public Boolean isUndef();
    }

Implementation example: CoreMaterial.java

    package net.woopa.dungeon.core;
    import net.woopa.dungeon.datatypes.Material;

    public enum CoreMaterial implements Material {
UNDEF(' '), WALL('#'), FIXEDWALL('X'), UPWALL('U'), DOWNWALL('D'), BOTHWALL(
        'B'), PRESSURE('x'), RED1('1'), RED2('2'), ARROW('>'), FLOOR('.'), FIXEDFLOOR(
        ','), FIXEDFLOORUP(';'), FIXEDFLOORDOWN(':'), O_FLOOR('`'), WINDOW(
        'G'), BARS('I'), HIGH_BARS('b'), DOOR('+'), UP('^'), DOWN('V'), ARCH(
        'A'), HIDDEN('$'), WATER('W'), LAVA('L'), ANVIL('a'), FURNACE('f'), BOOKCASE(
        'k'), BOOKCASE2('K'), SIGNPOST('p'), ENCHANT('e'), TORCH('t'), O_TORCH(
        '~'), WEB('w'), SHROOM('m'), CAKE('='), SOULSAND('s'), EMPTYCHEST(
        'o'), CHEST('c'), MIDCHEST('C'), BIGCHEST('*'), WORKBENCH('T'), SPAWNER(
        'M'), BED_H('Z'), BED_F('z'), NONE('!');

private final char ch;

CoreMaterial(char ch) {
    this.ch = ch;
}

public Boolean isFloor() {
    return this == FLOOR || this == FIXEDFLOOR || this == FIXEDFLOORUP
            || this == FIXEDFLOORDOWN;
}

public Boolean isWall() {
    return this == CoreMaterial.WALL || this == CoreMaterial.FIXEDWALL
            || this == CoreMaterial.DOWNWALL
            || this == CoreMaterial.BOTHWALL || this == CoreMaterial.UPWALL;
}

public Boolean isChest() {
    return this == CoreMaterial.CHEST || this == CoreMaterial.MIDCHEST
            || this == CoreMaterial.BIGCHEST
            || this == CoreMaterial.EMPTYCHEST;
}

public Boolean isDoor() {
    return this == CoreMaterial.DOOR || this == CoreMaterial.ARCH
            || this == CoreMaterial.HIDDEN || this == CoreMaterial.WEB;
}

public Boolean isStair() {
    return this == CoreMaterial.UP || this == CoreMaterial.DOWN;
}

public Boolean isUndef() {
    return this == CoreMaterial.UNDEF;
}

public Boolean isBoundary() {
    return (this.isWall() || this.isStair() || this.isDoor());
}

@Override
public char getSymbol() {
    return ch;
}

@Override
public String getName() {
    return this.name();
}
    }

4. package net.woopa.dungeon.core;

    import net.woopa.dungeon.datatypes.Direction;
    import net.woopa.dungeon.datatypes.Grid;
    import net.woopa.dungeon.datatypes.Material;
    import net.woopa.dungeon.datatypes.Vector2D;

    public class StandardMethods {
public static void build_door(Direction dir, Vector2D loc, Material door,
        Grid g) {
    int wayin_x = loc.getX();
    int wayin_y = loc.getY();
    g.set(wayin_x, wayin_y, door);
    fixWall(dir.left_x(wayin_x), dir.left_y(wayin_y), g);
    fixWall(dir.right_x(wayin_x), dir.right_y(wayin_y), g);
    fixFloor(dir.backwards_x(wayin_x), dir.backwards_y(wayin_y), g);
    fixFloor(dir.forwards_x(wayin_x), dir.forwards_y(wayin_y), g);
}

public static void fixWall(int x, int y, Grid grid) {
    if (grid.get(x, y) == CoreMaterial.WALL
            || grid.get(x, y) == CoreMaterial.UNDEF)
        grid.set(x, y, CoreMaterial.FIXEDWALL);
}

public static void fixFloor(int x, int y, Grid grid) {
    if (grid.get(x, y) == CoreMaterial.FLOOR
            || grid.get(x, y) == CoreMaterial.UNDEF)
        grid.set(x, y, CoreMaterial.FIXEDFLOOR);
}

public static void startUpStaircase(int x, int y, CoreMaterial up,
        Grid grid, Direction dir) {
    grid.set(x, y, CoreMaterial.UP);
    grid.set(dir.forwards_x(x), dir.forwards_y(y),
            CoreMaterial.FIXEDFLOORUP);
    upWall(dir.backwards_x(x), dir.backwards_y(y), grid);
    upWall(dir.left_x(x), dir.left_y(y), grid);
    upWall(dir.right_x(x), dir.right_y(y), grid);
}

public static void upWall(int x, int y, Grid grid) {
    if (grid.get(x, y) == CoreMaterial.DOWNWALL) {
        grid.set(x, y, CoreMaterial.BOTHWALL);
    } else if (grid.get(x, y) == CoreMaterial.WALL
            || grid.get(x, y) == CoreMaterial.FIXEDWALL) {
        grid.set(x, y, CoreMaterial.UPWALL);
    } else if (grid.get(x, y) == CoreMaterial.UNDEF) {
        grid.set(x, y, CoreMaterial.UPWALL);
        grid.use();
    }
}
    }

CoreLevelGenerator.java package net.woopa.dungeon.core;

    import java.util.ArrayList;

    import net.woopa.dungeon.core.CoreRoom.RoomType;
    import net.woopa.dungeon.datatypes.Direction;
    import net.woopa.dungeon.datatypes.Grid;
    import net.woopa.dungeon.datatypes.LevelCreator;
    import net.woopa.dungeon.datatypes.Vector2D;

    public class CoreLevelCreator implements LevelCreator {
private Vector2D levelStart, levelEnd;
private Direction startDir, endDir;
private ArrayList<CoreRoom> rooms = new ArrayList<CoreRoom>();

@Override
public Vector2D levelStart() {
    return levelStart;
}

@Override
public Vector2D levelEnd() {
    return levelEnd;
}

@Override
public Grid generate(Vector2D levelSize, Vector2D startLocation,
        Direction startDirection) {
    Grid grid = new Grid(levelSize);
    this.levelStart = startLocation;
    CoreRoom start = new CoreRoom(grid);
    if (!start.startRoom(startLocation.getX(), startLocation.getY(),
            startDirection)) {
        // Couldn't place the starting room
        return null;
    }
    // start_dir = r.room_dir();
    rooms.add(start);
    CoreRoom n = new CoreRoom(grid);
    CoreRoom from = start;
    int max_gen = 0;
    // Place rooms until we have to back track too much
    for (int t = 0; t < 1000 && from != null; t++) {
        if (from.getExtensionAttempts() >= 400) {// TODO hardcoded int
            from = getRoomNearEnd();
        }
        if (from != null && n.nextRoom(from)) {
            rooms.add(n);
            if (n.getGen() > max_gen)
                max_gen = n.getGen();
            if (grid.percentUtilized() > 85.0) {// TODO hardcoded float
                from = null;
            } else {
                from = this.rooms.get(RandomUtil.nextInt(rooms.size()));
            }
            n = new CoreRoom(grid);
        }
    }
    // Find a location for the end room
    clearRoomAttempts();
    from = getRoomGen();
    for (int t = 0; t < 1000 && from != null; t++) {
        if (from.getExtensionAttempts() >= 400) {// TODO hardcoded int
            from = getRoomGen();
        }
        if (from != null && n.endRoom(from)) {
            levelEnd = n.wayin().clone();
            endDir = n.getRoomDir();
            from = null;
            n = new CoreRoom(grid);
        }
    }
    if (true && from == null) {
        // Fill the rest of the map
        from = getRoomNearEnd();
        for (int t = 0; t < 1000 && from != null; t++) {
            if (from.getExtensionAttempts() >= 400) {// TODO hardcoded int
                from = getRoomNearEnd();
            }
            if (from != null && n.nextRoom(from)) {
                rooms.add(n);
                from = n;
                n = new CoreRoom(grid);
            }
        }

        if (endDir == null) {
            // No end room
        }
    } else {
        // No start room
    }
    this.randomlyAddDoors(grid);
    return grid;
}

private CoreRoom getRoomGen() {
    CoreRoom r = null;
    int max = 0;
    if (rooms != null) {
        for (CoreRoom x : rooms) {
            if (!(x.getExtensionAttempts() >= 400)) {// TODO hardcoded int
                if (x.getGen() >= max && x.getType().equals(RoomType.ROOM)) {
                    r = x;
                    max = x.getGen();
                }
            }
        }
    }
    return r;
}

private void clearRoomAttempts() {
    if (rooms != null) {
        for (CoreRoom x : rooms) {
            x.clearAttempts();
        }
    }
}

private void randomlyAddDoors(Grid grid) {
    for (int x = 0; x < grid.getSize().getX(); x++) {
        for (int y = 0; y < grid.getSize().getY(); y++) {
            if (grid.get(x, y).isWall()) {
                Direction dir = Direction.randomDirection();
                if (grid.get(dir.left_x(x), dir.left_y(y)).equals(
                        CoreMaterial.WALL)
                        && grid.get(dir.right_x(x), dir.right_y(y)).equals(
                                CoreMaterial.WALL)
                        && grid.get(dir.backwards_x(x), dir.backwards_y(y))
                                .equals(CoreMaterial.FLOOR)
                        && grid.get(dir.forwards_x(x), dir.forwards_y(y))
                                .equals(CoreMaterial.FLOOR)) {
                    StandardMethods.build_door(dir, new Vector2D(x, y),
                            CoreMaterial.CAKE, grid);
                }
            }
        }
    }
}

private CoreRoom getRoomNearEnd() {
    CoreRoom r = null;
    if (rooms != null) {
        for (CoreRoom x : rooms) {
            if (!(x.getExtensionAttempts() >= 400)) { // TODO hardcoded int
                r = x;
            }
        }
    }
    return r;
}

@Override
public Direction endDirection() {
    return endDir;
}

@Override
public Direction startDirection() {
    return startDir;
}

public void clean(){
    this.rooms.clear();
}

    }

CoreRoom.java package net.woopa.dungeon.core;

    import net.woopa.dungeon.datatypes.Direction;
    import net.woopa.dungeon.datatypes.Grid;
    import net.woopa.dungeon.datatypes.Schematic;
    import net.woopa.dungeon.datatypes.Vector2D;
    import net.woopa.dungeon.managers.SchematicManager;
    import net.woopa.dungeon.managers.SettingsManager;

    public class CoreRoom {
public enum RoomType {
    ROOM, CORRIDOR, SPECIAL
}

private int size_x, size_y;
private int origin_x, origin_y;
private int gen = 0;
private int extension_attempts = 0;
private final Grid grid;
private RoomType type;
private Schematic room_map = null;
private Direction special_dir;

private final RoomPopulator roomPopulator;;

private int wayin_x, wayin_y;
private Direction room_dir;

public CoreRoom(Grid grid) {
    this.roomPopulator = new RoomPopulator(this);
    this.grid = grid;
    generateRandom();
    placeRandom();
}

public void clearAttempts() {
    extension_attempts = 0;
}

// TODO this is better but still horrible
public Boolean startRoom(int x, int y, Direction orig_dir) {
    int cnt = 0;
    Direction dir;
    do {
        dir = placeFrom(x, y, orig_dir);
        if (!grid.fits(origin_x, origin_y, size_x, size_y) && (dir != null)) {
            generateRandom();
        } else {
            break;
        }
        cnt++;
    } while (cnt < 5000);

    if (cnt >= 5000)
        // Can't find a place
        return false;
    renderRoom();
    StandardMethods.startUpStaircase(x, y, CoreMaterial.UP, grid, dir);
    gen = 1;
    roomPopulator.dressRoom();
    return true;
}

public Boolean nextRoom(CoreRoom from) {
    int cnt = 0;
    Direction dir = null;
    do {
        dir = placeFrom(from);
        if (!((dir != null)
                && grid.fits(origin_x, origin_y, size_x, size_y)
                && (grid.get(wayin_x, wayin_y) == CoreMaterial.WALL)
                && grid.isFloor(dir.backwards_x(wayin_x),
                        dir.backwards_y(wayin_y)))) {
            generateRandom();
        }else break;
        from.extension_attempts++;
        cnt++;
    } while ((cnt < 20)); // Try different sizes and shapes //TODO
    // hardcoded
    if (cnt>=20)
        // Can't find a place
        return false;

    renderRoom();
    StandardMethods.build_door(dir, new Vector2D(wayin_x, wayin_y),
            CoreMaterial.DOOR, grid);
    gen = from.gen + 1;
    roomPopulator.dressRoom();
    return true;
}

public Boolean endRoom(CoreRoom from) {
    this.type = RoomType.SPECIAL;
    generateSpecialRoomRandom(SchematicManager.randomDownSchematic());

    final Direction dir = placeFrom(from);
    final Boolean ok = (dir != null)
            && grid.fits(origin_x, origin_y, size_x, size_y)
            && (grid.get(wayin_x, wayin_y) == CoreMaterial.WALL)
            && grid.isFloor(dir.backwards_x(wayin_x),
                    dir.backwards_y(wayin_y));

    if (ok) {
        //extension_attempts = 100000; // TODO HC
        this.renderRoom();
        gen = from.gen + 1;
        if (grid.get(wayin_x, wayin_y) == CoreMaterial.DOWN) {
            grid.set(dir.backwards_x(wayin_x), dir.backwards_y(wayin_y),
                    CoreMaterial.FIXEDFLOORDOWN);
        } else {
            StandardMethods.build_door(dir, new Vector2D(wayin_x, wayin_y),
                    CoreMaterial.DOOR, grid);
        }
        roomPopulator.chestDoubleRandom();
    }
    return ok;
}

private Direction placeFrom(CoreRoom from) {
    final Direction dir = Direction.randomDirection();
    final int offset = randomOffset(from, dir);
    if (offset < 0)
        return null;

    switch (dir) {
    case EAST:
        wayin_x = (from.origin_x + from.size_x) - 1;
        wayin_y = from.origin_y + offset;
        break;
    case WEST:
        wayin_x = from.origin_x;
        wayin_y = from.origin_y + offset;
        break;
    case NORTH:
        wayin_x = from.origin_x + offset;
        wayin_y = (from.origin_y + from.size_y) - 1;
        break;
    case SOUTH:
        wayin_x = from.origin_x + offset;
        wayin_y = from.origin_y;
        break;
    }
    return this.placeFrom(wayin_x, wayin_y, dir);
}

private Direction placeFrom(int x, int y, Direction dir) {
    wayin_x = x;
    wayin_y = y;
    setRoomDir(dir);
    final int offset = randomOffset(this, dir);
    if (offset < 0)
        return null;

    if (dir.isHorizontal()) {
        origin_x = (dir == Direction.EAST) ? x : (x - size_x) + 1;
        origin_y = y - offset;
    } else {
        origin_y = (dir == Direction.NORTH) ? y : (y - size_y) + 1;
        origin_x = x - offset;
    }
    return dir;
}

public int corridorWidth() {
    if (RandomUtil.chance(SettingsManager
            .getInt(CoreSettings.CorridorW3Pct)))
        return 3;
    if (RandomUtil.chance(SettingsManager
            .getInt(CoreSettings.CorridorW3Pct)
            + SettingsManager.getInt(CoreSettings.CorridorW2Pct)))
        return 2;
    return 1;
}

private void generateCorridorRandom() {
    final int width = corridorWidth();
    if (RandomUtil.chance(50)) {
        size_x = randomCorridorSize() + 2;
        size_y = width + 2;
    } else {
        size_x = width + 2;
        size_y = randomCorridorSize() + 2;
    }
}

private void generateRandom() {
    if (RandomUtil.chance(SettingsManager.getInt(CoreSettings.CorridorPct))) {
        generateCorridorRandom();
        this.type = RoomType.CORRIDOR;
    } else {
        if (RandomUtil.chance(SettingsManager
                .getInt(CoreSettings.SpecialPct))) {
            generateSpecialRoomRandom(null);
            this.type = RoomType.SPECIAL;
        } else {
            generateRoomRandom();
            this.type = RoomType.ROOM;
        }
    }
}

private void generateRoomRandom() {
    size_x = randomRoomSize();
    size_y = randomRoomSize();

}

private void generateSpecialRoomRandom(Schematic s) {
    if (s == null) {
        room_map = SchematicManager.randomRoomSchematic();
    } else {
        room_map = s;
    }
    special_dir = Direction.randomDirection();
    size_x = room_map.sx(special_dir);
    size_y = room_map.sy(special_dir);
}

public int getExtensionAttempts() {
    return this.extension_attempts;
}

public int getGen() {
    return this.gen;
}

public Grid getGrid() {
    return this.grid;
}

public int getOriginX() {
    return this.origin_x;
}

public int getOriginY() {
    return this.origin_y;
}

public Direction getRoomDir() {
    return room_dir;
}

public int getSizeX() {
    return this.size_x;
}

public int getSizeY() {
    return this.size_y;
}

public RoomType getType() {
    return type;
}

private void placeRandom() {
    if ((((grid.getSize().getX() - size_x) + 1) < 1)
            || (((grid.getSize().getY() - size_y) + 1) < 1)) {
        origin_x = 0;
        origin_y = 0;
    } else {
        origin_x = RandomUtil.nextInt((grid.getSize().getX() - size_x) + 1);
        origin_y = RandomUtil.nextInt((grid.getSize().getY() - size_y) + 1);
    }
}

public int randomCorridorSize() {
    final int cmax = SettingsManager.getInt(CoreSettings.CorridorMax);
    final int cmin = SettingsManager.getInt(CoreSettings.CorridorMin);
    return RandomUtil.nextInt((cmax - cmin) + 1) + cmin;
}

private int randomOffset(CoreRoom from, Direction dir) {
    int size = 0;
    if (dir.isHorizontal()) {
        size = from.size_y - 2;
    } else {
        size = from.size_x - 2;
    }
    return (from.getType().equals(RoomType.SPECIAL)) ? from.room_map
            .getAccess(dir, from.special_dir)
            : RandomUtil.nextInt(size) + 1;
}

private int randomRoomSize() {
    final int rmax = SettingsManager.getInt(CoreSettings.RoomMax);
    final int rmin = SettingsManager.getInt(CoreSettings.RoomMin);
    return RandomUtil.nextInt((rmax - rmin) + 1) + rmin + 2;
}

private void renderRoom() {
    if (type == RoomType.SPECIAL) {
        grid.renderSchematic(origin_x, origin_y, room_map, special_dir);
    } else {
        grid.renderBasicEmptyRoom(origin_x, origin_y, size_x, size_y,
                CoreMaterial.WALL, CoreMaterial.FLOOR);
    }
}

public void setRoomDir(Direction room_dir) {
    this.room_dir = room_dir;
}

@Override
public String toString() {
    return "ROOM:(" + origin_x + "," + origin_y + ") size(" + size_x + ","
            + size_y + ") gen=" + gen + " att=" + extension_attempts;
}

public Vector2D wayin() {
    return new Vector2D(wayin_x, wayin_y);
}
   }

Further study here: See here for source: https://github.com/samkio/Dungeonator

\$\endgroup\$
  • \$\begingroup\$ I took a look at your Direction code on GitHub, and I would suggest that you create a new question about just that class. I have a whole lot of things to say about that one :) \$\endgroup\$ – Simon Forsberg Feb 25 '14 at 17:57
  • \$\begingroup\$ Thanks, I've created a new question here: codereview.stackexchange.com/questions/42817/… \$\endgroup\$ – Sam Martin Feb 25 '14 at 21:56
6
\$\begingroup\$
  • You have some methods that return Boolean, prefer the primitive type boolean instead. Boolean can be null, boolean can't.

  • I would make a more flexible solution than Uri's for your material types, (although Uri's suggestion is a good start). Either use an array of MaterialType or use an EnumSet of MaterialType. (If you choose to construct it with an array, it can be converted to an EnumSet later, such as in the constructor).

    public enum CoreMaterial implements Material {
         UNDEF(' ', EnumSet.of(MaterialType.Undef, MaterialType.SomethingElse)), 
         WALL('#', EnumSet.of(MaterialType.Wall, MateiralType.Other)), ...;
    

This has the benefit that one Material can have multiple MaterialTypes. Then you can use the .contains method of the enumset to check if the material is of a specific type. See more in the EnumSet documentation

  • It is a horrible place to put a linebreak after BOTHWALL(. Put a linebreak after a comma , instead. Also try to put the enums that are related to each other on the same line so that it is easier to see which ones belongs together.

  • TODO hardcoded float. That tells me that you know what to do about this. Do it. Make it a constant. Or a variable. You have a whole bunch of hardcoded values. Not good. In some methods you even have the same hardcoded value at two places. Not not good. Imagine if you would change one and forget the other. Chaos!

  • if (true && from == null) { I'm quite sure that true will always be true, so no need to check that. If you keep it for easy disabling, make it a constant. SHOULD_FILL_REST_OF_MAP or something (you can probably come up with a better name).

  • Is this meant as some kind of // TODO ? If so, write // TODO. If not, remove this piece of code:

    if (endDir == null) {
        // No end room
    }
    
  • for (CoreRoom x : rooms) { and what on earth is x? room would be a better name but that would still not explained what it is being used for. The getRoomGen method is quite unclear. You might want to use better variable names or add some comments about why it exists.

  • You might want to extract a method out of randomlyAddDoors, one method can do the iteration and one method can do what needs to be done for one specific door.

As for your specific questions:

1 Firstly I used a Visitor Pattern for the Materials, allowing users to create their own materials using an enum that can be used within their project with ease. Is this intuitive for the programmer?

Allowing users to create their own materials is a good idea. I like your Material interface (although I think it needs to be modified to allow for more flexibility for material types, and you should return boolean as I mentioned...). Visitor pattern? I wouldn't call what you have here a visitor pattern. I think the most intuitive would be to specify which MaterialTypes a material should have when constructing it. In fact, you might want to have Material as a class instead, and the char and MaterialType array/enumset as parameters to the constructor.

2 Following this is the API easy to pickup even with little documentation?

It's quite easy, but I think it could use a bit more documentation. And I also think that it's not entirely clear which parts are your API and which parts is your implementation here.

3 Is the performance reduced in any way by writing it as shown.

I'm the wrong person to ask about performance :) Can't comment much on this one. I can't see any big performance issues directly though, which is a good sign.

4 I have a blend of object and static classes, would it be best for the user to have them all static? (Or alternatively all objects)?

I don't see many static classes, I only see static methods. If you mean the static methods, don't ever consider having them all static again. Not under my watch. Java is an object-oriented language. Use objects and classes. Learn to love the objects. Some utility methods can be static if you really want them to be. But here I see not much reason for them to be.

5 I always try good programming practices (for instance not including hard coded strings and ints) now there are some in there but I plan on moving them soon. Do you feel that this code achieves this with the enums for Materials, Schematics etc?

I like what you have done with the Materials. I'm not sure which Schematics you're talking about. And please make constants of your hardcoded values ASAP.

6 Moving to the implementation (net.woopa.dungeon.core) is this as efficient as it could be, I feel like it can be difficult to read in places.

A method should do only one thing, and it should do that one thing well. Yes, it is difficult to read, mostly because you have some unnecessary long methods. Split these long methods into multiple methods. Don't do more than you have to in the same method. For example, the generation process can be split into several sub-steps, make each sub-step it's own method (at least).

I hope that this helps.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your suggestions and advice. I like your extended flexibility of the Material Enum and will take that on board. As from both answers I will stop being lazy and leaving all the hardcoded ints in! Yes I meant static methods and I will move away from them quickly. Thanks for all the suggestions it is much appreciated! \$\endgroup\$ – Sam Martin Feb 25 '14 at 23:01
6
\$\begingroup\$

Keep your code extendable - You created your enum CoreMaterial as an extendable enum, but then you implemented the various isXXX() methods repeating the enum values themselves in them. A better solution might be to let each material declare its own type:

public enum MaterialType { Chest, Wall, Floor, Boundary, Undef }

public enum CoreMaterial implements Material {
UNDEF(' ', MaterialType.Undef), WALL('#', MaterialType.Wall), 
FIXEDWALL('X', MaterialType.Wall), UPWALL('U', MaterialType.Wall); // etc...

private final char ch;
private final MaterialType type;

CoreMaterial(char ch, MaterialType type) {
    this.ch = ch;
    this.type = type;
}

public Boolean isFloor() {
    return type == MaterialType.Floor;
}

public Boolean isWall() {
    return type == MaterialType.Wall;
}

public Boolean isChest() {
    return type == MaterialType.Chest;
}

// etc...
}

Naming conventions from time to time your code conventions slip - build_door, special_dir, etc. if you write in java, adhere to its conventions - buildDoor, specialDir, etc.

When static methods scream 'refactor me!' - in your StandardMethods class you have many static classes. Even the name of the class hints ti the fact that it is not very object-oriented...

When you look at all the methods in this class, you may note that all of them have one parameter in common, and they keep moving it around among themselves - g. This suggests that it may be a good idea to refactor all of them into the Grid class...

Don't override member names - you use levelStart and levelEnd to both the private members and their method getters. Better naming for the methods would be getLevelStart() and getLevelEnd()

generate() - TL;DR - you might want to break this method to smaller ones.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your suggestions. I will certainly take them on board and fix accordingly. I will also try and refactor the StandardMethods class. The reason they're not in the Grid class is that the StandardMethods are specific to that level generator whereas the Grid class should be universal and not have any set methods. Which I guess gives me some thought as to what to include in that class and what to exclude from it. \$\endgroup\$ – Sam Martin Feb 25 '14 at 22:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.