13
\$\begingroup\$

I have a heading, which is an integer and from it I would like to figure out the compass direction (North, North-East,...) and return the appropriate icon. I feel like I'm probably already at the cleanest solution but I would love to be proven wrong.

public GetHeadingImage(string iconName, int heading){
        if (IsNorthHeading(heading))
            return iconName  + "n" + ICON_FILE_EXTENTION;
        if (IsNorthEastHeading(heading))
            return iconName + "ne" + ICON_FILE_EXTENTION;
        if (IsEastHeading(heading))
            return iconName + "e" + ICON_FILE_EXTENTION;
        /*A bunch more lines*/
}
private bool IsNorthHeading(int heading)
{
    return heading < 23 || heading > 337;
}
/* A bunch more extracted test methods*/
\$\endgroup\$
  • \$\begingroup\$ Is it possible at all that the thing, whatever it may be, is not moving at all? \$\endgroup\$ – Leonid Aug 9 '12 at 21:30
26
\$\begingroup\$

A simple solution is a good solution:

public GetHeadingImage(string iconName, int heading){
    var directions = new string[] {
        "n", "ne", "e", "se", "s", "sw", "w", "nw", "n"
    };

    var index = (heading + 23) / 45;
    return iconName + directions[index] + ICON_FILE_EXTENSION;
}
\$\endgroup\$
  • 9
    \$\begingroup\$ +1 good and simple solution. Maybe its a good idea to replace the magic number ´45´ with ´int DegreesPerDirection = 360 / (directions.Length - 1)´ and ´23´ with ´DegreesPerDirection / 2´ \$\endgroup\$ – k3b Mar 15 '11 at 9:53
  • 2
    \$\begingroup\$ The index calculation seems solid, but a simple solution isn't a reuseable solution. I would still introduce an enum to represent the heading, as well as at least document the calculation, e.g. by removing the magic numbers as k3b mentions. \$\endgroup\$ – Steven Jeuris Mar 15 '11 at 10:58
  • \$\begingroup\$ @k3b: Good call. @Steven: I'd introduce a Direction enum if there places where it was used. In this case all we're visibly using are degrees, so I'd leave it out. We can always add it later if we actually need it. \$\endgroup\$ – munificent Mar 16 '11 at 1:44
  • \$\begingroup\$ Same calculation in JS, with more compass points: Is there a better way to get a compass point from 140° than a series of if's? \$\endgroup\$ – Danny Beckett Jan 5 '14 at 9:24
7
\$\begingroup\$

Small suggestion: the GetHeadingImage() has a lot of duplicate code.
Why not something like:

public GetHeadingImage(string iconName, int heading){
    return iconName  + GetHeadingName(heading) + ICON_FILE_EXTENTION;
}

You could then have the heading logic just inside GetHeadingName().

\$\endgroup\$
  • 1
    \$\begingroup\$ Combining this with the original code makes for a more readable result than the shorter but more cryptic suggestion. In six months this will still make sense. \$\endgroup\$ – Josh Earl Mar 16 '11 at 12:33
3
\$\begingroup\$

Two things: 1) Extract GetCompasDirection as a separate method which will return an enum 2) Create a collection of angles and corresponding headers to remove a lot of Is...Heading methods:

public enum CompasDirection
{
    North,
    NorthEast,
    // other directions
}

public CompasDirection GetCompasDirection(int heading)
{
    // directions in clock-wise order:
    var directionUpperLimitAngles = new [] {
         Tuple.Create(CompasDirection.North, 22),
         Tuple.Create(CompasDirection.NorthEast, 67),
         Tuple.Create(CompasDirection.East, 112),
         // other directions,
         Tuple.Create(CompasDirection.North, 360), // north again
    };

    return directionUpperLimitAngles.Last(d => d.Item2 <= heading).Item1;
}

public string GetHeadingImage(string imageName, int heading)
{
    var directionToIconSuffixMapping = new Dictionary<CompasDirection, string> {
        { CompasDirection.North, "n"},
        { CompasDirection.NorthEast, "ne"},
        // other directions
    };
    var direction = GetCompasDirection(heading);
    return iconName + directionToIconSuffixMapping[direction] + ICON_FILE_EXTENTION;
}

Some parts here can be simplify (for example you can remove second dictionary and simply name your icon files correspondingly to enum members).

This approach with direction-heading table if I remember correctly I've taken from McConnel's Code Complete

UPDATE: replaced inner private class with Tuples

\$\endgroup\$
  • 1
    \$\begingroup\$ What use is the enum here? Why not simply map numbers to suffixes? \$\endgroup\$ – pdr Mar 14 '11 at 21:34
  • 1
    \$\begingroup\$ @pdr, it can be mapped directly to suffixes, but only if you are sure that you will never-never-never need the directions again. Otherwise leaving them as suffixes will lead to stringly-typed code. \$\endgroup\$ – Snowbear Mar 14 '11 at 21:37
  • \$\begingroup\$ @Snowbear, Good point \$\endgroup\$ – pdr Mar 15 '11 at 1:44
  • \$\begingroup\$ +1, the main point is mentioned here, separate logic from representation! The enum is a must. Also, if the class CompassDirectionAngle isn't to be used outside of the class (it's correctly set to private), I would use a Tuple<CompassDirection, int> array instead, so the class can be removed. Giving the array a proper name is sufficient enough, the class only adds a negligible bit of readability. \$\endgroup\$ – Steven Jeuris Mar 15 '11 at 10:05
  • \$\begingroup\$ @Steven, I also thought about Tuples but they didn't fit into CR screen width :). But I agree, Tuples will look good here. \$\endgroup\$ – Snowbear Mar 15 '11 at 10:11
2
\$\begingroup\$

Another alternative:

public enum Direction
{
    North = 0,
    NorthEast = 1,
    East = 2,
    SouthEast = 3,
    South = 4,
    SouthWest = 5,
    West = 6,
    NorthWest = 7
}

public static class DirectionExtensions
{
    private static readonly Dictionary<Direction, string>
        mapping = new Dictionary<Direction, string>
    {
        { Direction.North, "n" },
        { Direction.NorthEast, "ne" },
        { Direction.East, "e" },
        { Direction.SouthEast, "se" },
        { Direction.South, "s" },
        { Direction.SouthWest, "sw" },
        { Direction.West, "w" },
        { Direction.NorthWest, "nw" }
    };

    public static bool IncludesHeading(this Direction direction, int heading)
    {
        var adjusted = (heading + 22) % 360;
        var adjMin = (int) direction * 45;
        var adjMax = adjMin + 44;
        return (adjusted >= adjMin && adjusted <= adjMax);
    }

    public static string GetSuffix(this Direction direction)
    {
        return mapping[direction];
    }
}

Leaves your method reading like this:

public string GetHeadingImage(string imageName, int heading)
{
     Direction[] directions = ((Direction[]) Enum.GetValues(typeof(Direction)));
     var match = directions.First(d => d.IncludesHeading(heading));
     return imageName + match.GetSuffix() + ICON_FILE_EXTENTION;
}

[Edit: Taking that one step further]

Replace the IncludesHeading extension with

public static IntDirectionExtensions
{
    public static Direction GetDirection(this int heading)
    {
        var adjusted = (heading + 22) % 360;
        var sector = adjusted / 45;
        return (Direction)sector;
    }
}

And now you can simplify your method to

public string GetHeadingImage(string imageName, int heading)
{
    return imageName + heading.GetDirection().GetSuffix() + ICON_FILE_EXTENTION;
}

[Edit 2: Another idea]

Another thing you could do is map to the suffix via reflection, which I think looks nicer but is probably less efficient

public enum Direction
{
    [IconSuffix("n")] North = 0,
    [IconSuffix("ne")] NorthEast = 1,
    [IconSuffix("e")] East = 2,
    [IconSuffix("se")] SouthEast = 3,
    [IconSuffix("s")] South = 4,
    [IconSuffix("sw")] SouthWest = 5,
    [IconSuffix("w")] West = 6,
    [IconSuffix("nw")] NorthWest = 7
}

public class IconSuffixAttribute : Attribute
{
    public string Suffix { get; private set; }
    public IconSuffixAttribute(string suffix)
    {
         Suffix = suffix;
    }
}

Replacing your GetSuffix extension (and now-defunct Dictionary mapping) with

public static string GetSuffix(this Direction direction)
{
    var suffix = from m in typeof(Direction).GetMember(direction.ToString())
                 from a in m.GetCustomAttributes(typeof(IconSuffixAttribute), false)
                 select ((IconSuffixAttribute) a).Suffix;
    return suffix.First();
}

Everything else remains the same.

\$\endgroup\$
  • \$\begingroup\$ I don't really find an extension method appropriate here. \$\endgroup\$ – Steven Jeuris Mar 15 '11 at 10:29
  • \$\begingroup\$ @Steven, any reason for that? I find it makes the GetHeadingImage method more readable. But honestly, that's not the main part of the solution (static methods are equally effective); it's the maths vs lookup that is different from the other solutions (or was, at the time I wrote it). \$\endgroup\$ – pdr Mar 15 '11 at 10:33
  • \$\begingroup\$ IMHO, I would expect such a method to be placed in a Compass class (or in java in the enum itself?), and be called from there. Otherwise the coupling just feels wrong. The list of extension methods for int would be infinite. Personally, I only use extension methods where I would otherwise have to write a helper class because I can't modify the source of the original class. Furthermore, the math is confusing, includes unnecessary magic numbers, and I believe is more complex than Snowbear's simple lookup. \$\endgroup\$ – Steven Jeuris Mar 15 '11 at 10:46
  • \$\begingroup\$ @Steven, Ok, I disagree, but at least I understand now. Extension methods don't generally get out of hand because you limit access to them by namespace, and int is a perfect example of a class I have no access to while an enum can't have functionality. As for the magic numbers; I think Snowbear's are just hidden. Consider the directionAngles array written out in full. Any one of those numbers could be wrong and cause a subtle bug. \$\endgroup\$ – pdr Mar 15 '11 at 10:53
  • \$\begingroup\$ I should point out here (in the midst of seeming criticism) that I voted up Snowbear's answer myself. I prefer it to the others by a long shot. \$\endgroup\$ – pdr Mar 15 '11 at 10:54
2
\$\begingroup\$

You could simplify your solution by concatenating the north/south letter with the east/west letter and thus avoid any need for IsNorthEastHeading and such like.

string northsouth = (heading < 23 || heading > 337) ? "n" :
                    (heading > 157 && heading < 203) ? "s" : 
                    "";
string eastwest = ...
return iconName + northsouth + eastwest + ICON_FILE_EXTENTION;

Is it really worth adding all those extra methods or introducing enums? Personally, I prefer this three line method over all of the much larger solutions proposed.

\$\endgroup\$
  • \$\begingroup\$ Hey! This code borrows heavily from my patented FizzBuzz algorithm implementation. My lawyer will get in touch with you. \$\endgroup\$ – Leonid Aug 9 '12 at 21:26
1
\$\begingroup\$

Naming (minor): Something possibly more specific to the domain for sector boundaries ('left'? and 'right'? relative angles) might be instead be called a radial. Each radial in at least [aeronautical] navigation is referred to as the '#named number of radial# radial', such as 'the 025 radial', or commonly just by number, 'the 025' (read as zero two five). Perhaps this would help minimize magic numbers by declaring your boundaries as named radial constants.

To go a step further, since you are dividing the compass into equally sized parts, or partitions, you might create constant/immutable value objects that describe these partitions. 'CardinalDirection' (n e s w) with public getters of left radial and right radial is an revised offhand suggestion. Ordinals are the next set of directional divisions (ne se sw nw).

Hope this helps refine your model for the better.

\$\endgroup\$
-2
\$\begingroup\$

A formula that gives the 8-way compass directions. Is working for any value of X and Y .For X=Y=0 the result is undefined. I write the formula in the same way ,that i did in excel.

f(X,Y)=MOD(2*(2-SIGN(Y1)-(1+SIGN(X1))*(1-SIGN(Y1^2)))-SIGN(X1*Y1)-0.5*SIGN(Y1*X1^3-X1*Y1^3)

  *(1+SIGN(ABS(ABS(X1)-ABS(Y1))/(ABS(X1)+ABS(Y1))-2^0.5+1-2^-22)),8) 

The formula gives for the East =0 and NE=1,N=2,NW=3,W=4,SW=5,S=6 and SE=7.

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. (Note: some of the existing answers are very old, and don't represent current best practice for answering!) \$\endgroup\$ – Toby Speight Feb 26 '19 at 15:47

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