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

3 Answers 3

up vote 6 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? –  awashburn yesterday
4  
@awashburn Because in my opinion, nested ternaries reduce readability. –  Simon André Forsberg yesterday

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 yesterday
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 yesterday
    
While appreciated, it's not the answer to my question. –  Vapid Linus yesterday
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 yesterday
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 yesterday

You can always go for branchless code:

// 1 = negative
// 0 = positive
public static int isNegative(float val) {
    return (Float.floatToIntBits(val)) >>> 31;
}

public enum Direction {
    NORTH_EAST, NORTH_WEST, SOUTH_EAST, SOUTH_WEST;
    //I had to reorder the directions (or use index mapping)

    public static Direction vectorToDirection(Vector2 vector) {
        int index = 2 * isNegative(vector.y) + isNegative(vector.x);
        return Direction.values()[index];
    }
}
share|improve this answer
1  
We're looking for answers that are more than just code snippets. Provide some additional explanation of your method. –  mleyfman 22 hours ago
    
nice hackish/golf code, Not sure if faster than OP's code(or of other answers). –  Bhathiya-JaDogg-Perera 20 hours ago
    
The mathematics is a neat trick just for this specific 4-option case, but like what @Bhathiya-JaDogg-Perera said, it's more like a code golf and thus I will even consider doing away with the clumsy isNegative() implementation, going for something simpler like values()[2 * (vector.y < 0 ? 1 : 0) + (vector.x < 0 ? 1 : 0)]. What I dislike is the assumption on the ordering of enum values, I will be sure to document that very carefully if I am picking this solution. –  h.j.k. 16 hours ago

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.