Vector-to-direction method in Java

Question

I don't really have a problem, but my code bothers me, because I know it could be done in a much better way.

I have an enum of 4 directions: north west, north east, south east and south west. And I have a method that converts a vector to one of those directions. The method prefers North and East, so values of 0 would return those.

public enum Direction {
 NORTH_WEST, NORTH_EAST, SOUTH_EAST, SOUTH_WEST;
 public static Direction vectorToDirection(Vector2 vector) {
 if (vector.y >= 0) {
 if (vector.x >= 0) return NORTH_EAST;
 else return NORTH_WEST;
 } else {
 if (vector.x >= 0) return SOUTH_EAST;
 else return SOUTH_WEST;
 }
 }
}

If anyone can figure out a cleaner or more efficient method, I'd love to know!

Answer 1

Efficiency

I don't think that can be improved. You're only using two if-statements. This is as efficient as it can get, I believe.

Cleaner

Depends on if you consider the conditional operator ?: clean or not.

public static Direction vectorToDirection(Vector2 vector) {
 if (vector.y >= 0) {
 return vector.x >= 0 ? NORTH_EAST : NORTH_WEST;
 } else {
 return vector.x >= 0 ? SOUTH_EAST : SOUTH_WEST;
 }
}

This code does exactly the same as your previous code, it's just a different way of writing it.

In my opinion, it would be foolish to try and reduce it more than this. Be happy with this one. Good job.

• recursion.ninja: If ternary statements make the code it cleaner, why not nest them to remove the if/else & have a single return?
• Simon Forsberg: @awashburn Because in my opinion, nested ternaries reduce readability.

Answer 2

Use Braces:

Consider using braces for more readable code:

public static Direction vectorToDirection(Vector2 vector) {
 if (vector.y >= 0) {
 if (vector.x >= 0) return NORTH_EAST;
 else return NORTH_WEST;
 } else {
 if (vector.x >= 0) return SOUTH_EAST;
 else return SOUTH_WEST;
 }
}

Becomes (with eclipse Ctrl + Shift + F):

public static Direction vectorToDirection (Vector2 vector) {
 if (vector.y >= 0) {
 if (vector.x >= 0) {
 return NORTH_EAST;
 } else {
 return NORTH_WEST;
 }
 } else {
 if (vector.x >= 0) {
 return SOUTH_EAST;
 } else {
 return SOUTH_WEST;
 }
 }
}

And suddenly it becomes extremely obvious and readable what happens here.

Naming:

Additionally I'd consider naming your method fromVector() instead. Compare:

Direction.vectorToDirection(someVector);

against

Direction.fromVector(someVector);

Documentation:

When I see the implementation I know, but how would anyone outside know you prefer east > west and north > south? Write some JavaDoc. Example:

/**
* @param vector the 2-dimensional vector to be converted to a direction
* @returns The Direction. hereby North is preferred over South and 
* East is preferred over West. 
* Example: Direction.fromVector(new Vector2(0, 0)); will return NORTH_EAST
* Direction.fromVector(new Vector2(-1, 0)); will return SOUTH_EAST
*/
public static Direction fromVector(Vector2 vector) {
// ...

• Ben Aaronson: I think the style of using braces vs. in-line for if statements with only a single line is mostly a matter of taste. The "bug-prone" really only comes in if you put it on a new line, without braces. In-line with braces is also perhaps slightly more readable.
• Vogel612: @Ben it may be my personal preference, but I find inlined if-blocks unreadable to the point of declaring it a code-obfuscation. It's so much harder to read the intent of these than if they are at least properly indented. And from that it's only a short step to add the braces, just to be safe and consistent.
• Vogel612: @VapidLinus in what way is this not "cleaner" than your original solution? Is it not more obvious, don't the names fit the use-case more? What did you want to hear? Feel free to come to Code Review Chat
• Chris Cirefice: I disagree with the 'braces' section for this specific case - the ternary solution reduces the (visible) nesting of conditionals, which is much cleaner - especially because this code is unlikely to change, except with maybe the return values switching if the OP decides at some point that west > east.