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\$
1
  • \$\begingroup\$ Is it possible at all that the thing, whatever it may be, is not moving at all? \$\endgroup\$
    – Leonid
    Commented Aug 9, 2012 at 21:30

7 Answers 7

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\$
4
  • 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
    Commented Mar 15, 2011 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\$ Commented Mar 15, 2011 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
    Commented Mar 16, 2011 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\$ Commented Jan 5, 2014 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
  • 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
    Commented Mar 16, 2011 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\$
7
  • 1
    \$\begingroup\$ What use is the enum here? Why not simply map numbers to suffixes? \$\endgroup\$
    – pdr
    Commented Mar 14, 2011 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
    Commented Mar 14, 2011 at 21:37
  • \$\begingroup\$ @Snowbear, Good point \$\endgroup\$
    – pdr
    Commented Mar 15, 2011 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\$ Commented Mar 15, 2011 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
    Commented Mar 15, 2011 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\$
5
  • \$\begingroup\$ I don't really find an extension method appropriate here. \$\endgroup\$ Commented Mar 15, 2011 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
    Commented Mar 15, 2011 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\$ Commented Mar 15, 2011 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
    Commented Mar 15, 2011 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
    Commented Mar 15, 2011 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\$
1
  • \$\begingroup\$ Hey! This code borrows heavily from my patented FizzBuzz algorithm implementation. My lawyer will get in touch with you. \$\endgroup\$
    – Leonid
    Commented Aug 9, 2012 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\$
1
  • 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\$ Commented Feb 26, 2019 at 15:47

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