Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

As request of Simon's comment:

Direction.java

package net.woopa.dungeon.datatypes;

import java.util.Random;

public enum Direction {
    NORTH, EAST, SOUTH, WEST;
    private static Random rnd = new Random();

    static public Direction randomDirection() {
        return Direction.values()[rnd.nextInt(4)];
    }

    // Rotate 90 degrees clockwise
    public Direction rotate90() {
        return values()[(ordinal() + 1) % 4];
    }

    // Rotate 180 degrees
    public Direction rotate180() {
        return values()[(ordinal() + 2) % 4];
    }

    // Rotate 270 degrees clockwise (90 counterclockwise)
    public Direction rotate270() {
        return values()[(ordinal() + 3) % 4];
    }

    public Boolean isHorizontal() {
        return this == EAST || this == WEST;
    }

    public Boolean isVertical() {
        return this == NORTH || this == SOUTH;
    }

    public int dx(int steps) {
        if (this == EAST) {
            return steps;
        }
        if (this == WEST) {
            return -steps;
        }
        return 0;
    }

    public int dy(int steps) {
        if (this == NORTH) {
            return steps;
        }
        if (this == SOUTH) {
            return -steps;
        }
        return 0;
    }

    public int forwards_x(int n) {
        if (this == EAST) {
            return n + 1;
        }
        if (this == WEST) {
            return n - 1;
        }
        return n;
    }

    public int forwards_y(int n) {
        if (this == NORTH) {
            return n + 1;
        }
        if (this == SOUTH) {
            return n - 1;
        }
        return n;
    }

    public int backwards_x(int n) {
        if (this == EAST) {
            return n - 1;
        }
        if (this == WEST) {
            return n + 1;
        }
        return n;
    }

    public int backwards_y(int n) {
        if (this == NORTH) {
            return n - 1;
        }
        if (this == SOUTH) {
            return n + 1;
        }
        return n;
    }

    public int left_x(int n) {
        if (this == NORTH) {
            return n - 1;
        }
        if (this == SOUTH) {
            return n + 1;
        }
        return n;
    }

    public int left_y(int n) {
        if (this == EAST) {
            return n + 1;
        }
        if (this == WEST) {
            return n - 1;
        }
        return n;
    }

    public int right_x(int n) {
        if (this == NORTH) {
            return n + 1;
        }
        if (this == SOUTH) {
            return n - 1;
        }
        return n;
    }

    public int right_y(int n) {
        if (this == EAST) {
            return n - 1;
        }
        if (this == WEST) {
            return n + 1;
        }
        return n;
    }
}

What are your thoughts about this class? The main grip I have personally is the left_y, right_x, etc methods. The code works, and here is some example of its usage below. But I feel there may be a better way?

Example usage:

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);
}

The above example has a location (x,y) for a door on a grid and then checks around it to ensure that there are doors either side and floors the other two either sides?


Symbols:

. = floor

# = wall

+ = door

Example:

.#. 
.+.
.#.

This would be okay for the placing of the door.

share|improve this question
    
Great! I see many people have answered already. Unfortunately I don't have a lot of time at the moment but I plan on adding another answer tomorrow. –  Simon André Forsberg Feb 26 at 11:14

5 Answers 5

up vote 8 down vote accepted

There are two things I think worth mentioning here:

  • use of random
  • magic numbers in enums

Random

Even though the java.util.Random class is thread-safe, it is not a great idea to use them in a multi-threaded context. As your class is an enum, it is likely that at some point it may be used in a threaded context (if it is not already). You should consider changing your code:

private static Random rnd = new Random();

to instead be (note also that this is 'final', and this is a java.util.concurrent.ThreadLocalRandom):

private static final ThreadLocalRandom rnd = ThreadLocalRandom.current();

This will give you better scalability in the future (or the present if you are already multi-threaded.

Enum Magic Numbers

Enums are a great concept in Java, and, it is one of those places where I believe magic numbers (special values with little apparent context or meaning) are really useful, and are OK.

You can embed 'magic' in with the enums and not worry too much about the readability... enums are supposed to be magic things.... So, I would consider code like the following:

import java.util.concurrent.ThreadLocalRandom;

public enum Direction {

    // use magic numbers to set the ordinal (used for rotation),
    // and the dx and dy of each direction.
    NORTH(0, 0, 1),
    EAST(1, 1, 0),
    SOUTH(2, 0, -1),
    WEST(3, -1, 0);

    private static final ThreadLocalRandom rnd = ThreadLocalRandom.current();

    static public Direction randomDirection() {
        return Direction.values()[rnd.nextInt(4)];
    }

    private final int r90index, r180index, r270index;
    private final boolean horizontal, vertical;
    private final int dx, dy;

    private Direction(int ordinal, int dx, int dy) {
        // from the ordinal, dx, and dy, we can calculate all the other constants.
        this.dx = dx;
        this.dy = dy;
        this.horizontal = dx != 0;
        this.vertical = !horizontal;
        this.r90index  = (ordinal + 1) % 4; 
        this.r180index = (ordinal + 2) % 4; 
        this.r270index = (ordinal + 3) % 4; 
    }


    // Rotate 90 degrees clockwise
    public Direction rotate90() {
        return values()[r90index];
    }

    // Rotate 180 degrees
    public Direction rotate180() {
        return values()[r180index];
    }

    // Rotate 270 degrees clockwise (90 counterclockwise)
    public Direction rotate270() {
        return values()[r270index];
    }

    public Boolean isHorizontal() {
        return horizontal;
    }

    public Boolean isVertical() {
        return vertical;
    }

    public int dx(int steps) {
        return dx * steps;
    }

    public int dy(int steps) {
        return dy * steps;
    }

    public int forwards_x(int n) {
        return n + dx;
    }

    public int forwards_y(int n) {
        return n + dy;
    }

    public int backwards_x(int n) {
        return n - dx;
    }

    public int backwards_y(int n) {
        return n - dy;
    }

    public int left_x(int n) {
        // if we are E/W facing, our left/right 'x' co-ords are still 'x'
        // if we are N/S facing, our left/right 'x' is `-dy`
        return n - dy;
    }

    public int left_y(int n) {
        // see left_x comments for the idea
        return n + dx;
    }

    public int right_x(int n) {
        // see left_x comments for the idea
        return n + dy;
    }

    public int right_y(int n) {
        // see left_x comments for the idea
        return n - dx;
    }
}

The code above is very efficient. There are no conditionals in the methods, they will likely all be inlined in to the calling code. The 'smarts' are all contained within the enum, it 'just works'.

share|improve this answer
    
I like the efficiency of this class and the fact it doesn't require me to change any of the other code in the project. I did however add public Vector2D getFront(Vector2D vec) { return new Vector2D(this.forwards_x(vec.getX()), this.forwards_y(vec.getY()));}... as I liked 200_success's naming convention, the simplicity and it didn't have any noticable drop in performance. –  Sam Martin Feb 27 at 12:15

It is better to replace

return Direction.values()[rnd.nextInt(4)];

with something like

return Direction.values()[rnd.nextInt(Direction.values().length)];

for it to not break if you decide to add something like NORTH_EAST.

I don't understand what functions like dx(int steps) are supposed to do, but it could be much easier to represent a direction as a vector. For example, rotation becomes simple multiplication, your methods are much more readable and robust. Also I don't understand why do you not use a Point-like class.

Also it is not logical for a direction to house its very own object of Random class.

share|improve this answer
    
dx(int steps) requires a bit more context. Think of a 2D grid of points. I have a point say (x,y) and a direction, x is the horizontal plane and y in the vertical plane. dx(int steps) will move my (x,y) point that many steps in the x direction. Thus if the direction is in the vertical plane (North/South) it won't move my value of x. Writing this comment has made me realize that yes using a vector will be so much simpler... I will switch to this as suggested. Thanks for the suggestions I will move Random too. –  Sam Martin Feb 25 at 23:06

An enum is a nice simple way to represent cardinal directions. No problem there. All the functions you've provided are valuable to consumers. No problem there.

But within the class, when you implement all those functions, the enum is getting in the way and making things complicated. The enum is there for your users, not for you. I would suggest you internally use a representation that makes more sense, like 2D coordinates. You have a lot of math that will make more sense with 2D coordinates.

A simple way to do that is to translate enums to coordinates for internal math, and back to enums for results.

private int[][] coordinates = new int[][] {
  new int[] {0,1,  0},  // NORTH (the third element maps back to the enum)
  new int[] {1,0,  1},  // EAST
  new int[] {0,-1, 2},  // SOUTH
  new int[] {-1,0, 3}   // WEST
};
public int[] toCoordinates(Direction d) {
  return coordinates[d.ordinal()];
}
public static Direction coordinatesToEnum(int[] c) {
  return Direction.values(c[2]);
}

public Boolean isVertical() {
  return this.toCoordinates()[1] == 0;
}
public Direction rotate180() {
  int[] c = this.toCoordinates();
  int result = new int[] { c[0]*-1, c[1]*-1 };
  return coordinatesToEnum(result);
}
// and so forth.

(This also allows your users to convert easily to coordinate system if that's better for them. Up to you whether you document that.)

share|improve this answer

I don't think that a Direction enum is sufficient to model your problem. Direction should just be a list of four cardinal directions. What you need is a Cursor or Character object that represents a position and heading on a grid. (Unfortunately, Character clashes with java.lang.Character, so you'll need to pick a different name.)

Your code should look more or less like this. Design your Cursor class accordingly to make it work.

Cursor cur = new Cursor(grid, x, y, Direction.random());
if ( CoreMaterial.WALL.equals(cur.getLeft()) &&
     CoreMaterial.WALL.equals(cur.getRight()) &&
     CoreMaterial.FLOOR.equals(cur.getBack()) &&
     CoreMaterial.FLOOR.equals(cur.getFront()) ) {
    StandardMethods.buildDoor(cur, CoreMaterial.CAKE);
}

When you're done, the Direction enum should be pretty minimal — just a list of four cardinal directions and a .random() static method.

share|improve this answer
    
I really like this idea of a cursor, to me it just makes more sense than the forward_x, left_y methods etc. The only problem I see compared with rolfl's answer is that it won't be as efficient as a result as you would have to create a new object everytime you wished to use it. Maybe I just need better naming conventions for rolfl's answer. –  Sam Martin Feb 27 at 10:02

Rename rotate90, rotate180, and rotate270 as right, reverse, and left, respectively. Don't use degrees unless you have to, and in this case they have nothing to do with this geometry system.

share|improve this answer
    
Furthermore, it's ambiguous whether rotate90 means "right" (compass heading convention) or "left" (mathematicians' convention). –  200_success Feb 26 at 11:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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