6
\$\begingroup\$

I'm working on a simple game in which I need to track the cardinal direction of an object. I experimented with using the enum's ordinal value, as well as using switches for the rotation, but both seemed wrong. This is what I ended up with. Is the following an adequate solution?

// Defines cardinal direction
public enum Direction {
    NORTH(0) {
        @Override
        public String getMessage() {
            return getDegrees() + " degrees due north";
        }
    },
    EAST(90) {
        @Override
        public String getMessage() {
            return getDegrees() + " degrees due east";
        }
    },
    SOUTH(180) {
        @Override
        public String getMessage() {
            return getDegrees() + " degrees due south";
        }
    },
    WEST(270) {
        @Override
        public String getMessage() {
            return getDegrees() + " degrees due west";
        }
    };

    private final int degrees;

    public abstract String getMessage();

    private Direction(final int degrees) {
        this.degrees = degrees;
    }

    public int getDegrees() {
        return degrees;
    }

    private static final Map<Integer, Direction> lookup = new HashMap<Integer, Direction>();

    static {
        for (Direction d : EnumSet.allOf(Direction.class))
            lookup.put(d.getDegrees(), d);
    }

    public static Direction get(int degrees) {
        return lookup.get(degrees);
    }

    public Direction rotateRight() {
        return Direction.get((degrees + 90) % 360);
    }

    public Direction rotateLeft() {
        return Direction.get((degrees + 270) % 360);
    }

}
\$\endgroup\$
6
\$\begingroup\$

Conceptually it is a great solution. I can suggest some changes, but in reality they are minor. It would also be 'fun' to tune it abit, but in the bigger picture the changes would be miniscule... still...

My biggest observation is that the constructors can be simplified a lot. Instead of having each Enum member create a unique method implementation (and that method builds a String each time - though the compiler will probably fix that), you could simplify it a lot with:

private final int degrees;
private final String message;

private Direction(final int degrees, final String name) {
    this.degrees = degrees;
    this.message = degrees + " degrees due " + name;
}

public int getDegrees() {
    return degrees;
}

public String getMessage() {
    return message;
}

With the change to the constructor, you have:

  1. no abstract methods to implement
  2. simple message which is a constant, instead of re-creating it each time you call getMessage()
  3. Your enums are initialized in a simpler way:

    NORTH(0, "North"),
    EAST(90, "East"),
    SOUTH(180, "South"),
    WEST(270, "West")
    

Apart from this, the code is pretty good, but, there is a way you can 'play' with the lookup system to use the ordinals only.... consider the following code (I'll leave it to you to figure out.... ;-):

public static Direction get(final int degrees) {
    int ordinal = ((degrees % 360) / 90) - ( 4 * (degrees % 90));
    return ordinal < 0 ?  null : values()[ordinal];
}    
\$\endgroup\$
  • \$\begingroup\$ @weare138 Just noticed a not-so-little bug in the lookups.... it would have returned North for any 0 <= degrees < 90 \$\endgroup\$ – rolfl Jan 3 '14 at 17:51

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.