17
\$\begingroup\$

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!

\$\endgroup\$

2 Answers 2

8
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ If ternary statements make the code it cleaner, why not nest them to remove the if/else & have a single return? \$\endgroup\$ Commented Aug 18, 2014 at 20:39
  • 5
    \$\begingroup\$ @awashburn Because in my opinion, nested ternaries reduce readability. \$\endgroup\$ Commented Aug 18, 2014 at 20:55
12
\$\begingroup\$

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) {
// ...
\$\endgroup\$
4
  • 4
    \$\begingroup\$ 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. \$\endgroup\$ Commented Aug 18, 2014 at 16:33
  • 2
    \$\begingroup\$ @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. \$\endgroup\$
    – Vogel612
    Commented Aug 18, 2014 at 16:36
  • 1
    \$\begingroup\$ @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 \$\endgroup\$
    – Vogel612
    Commented Aug 18, 2014 at 17:10
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ Commented Aug 18, 2014 at 21:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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