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.

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!

share|improve this question

2 Answers 2

up vote 7 down vote accepted

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.

share|improve this answer
    
If ternary statements make the code it cleaner, why not nest them to remove the if/else & have a single return? –  recursion.ninja Aug 18 at 20:39
4  
@awashburn Because in my opinion, nested ternaries reduce readability. –  Simon André Forsberg Aug 18 at 20:55

OUCH!

Use Braces!

It's not that much effort, and your code becomes more readable and less prone to bugs:

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

It's not like you're gonna run a static import on your Direction enum, right?

Documentation:

Well when I see the implementation I know, but... How is anyone outside supposed to 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) {
// ...
share|improve this answer
3  
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. –  Ben Aaronson Aug 18 at 16:33
2  
@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 Aug 18 at 16:36
    
While appreciated, it's not the answer to my question. –  Vapid Linus Aug 18 at 17:02
1  
@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 –  Vogel612 Aug 18 at 17:10
1  
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. –  Chris Cirefice Aug 18 at 21:32

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.