HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

API for Dungeon Generator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
generatorapifordungeon

Problem

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:

-
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?

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

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

-
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 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?

-
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'), ARR

Solution

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.

Code Snippets

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...
}

Context

StackExchange Code Review Q#42745, answer score: 6

Revisions (0)

No revisions yet.