Calculating compass direction
$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*/
c#
$endgroup$
add a comment |
$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*/
c#
$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
add a comment |
$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*/
c#
$endgroup$
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*/
c#
c#
edited 17 mins ago
Jamal♦
30.3k11119227
30.3k11119227
asked Mar 14 '11 at 20:18
stimmsstimms
20828
20828
$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
add a comment |
$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
$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
$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
add a comment |
7 Answers
7
active
oldest
votes
$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 aDirection
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
add a comment |
$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
add a comment |
$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 tostringly-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 classCompassDirectionAngle
isn't to be used outside of the class (it's correctly set to private), I would use aTuple<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 aboutTuples
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
|
show 2 more comments
$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
add a comment |
$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
add a comment |
$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$
add a comment |
$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.
New contributor
$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
13 hours ago
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f1287%2fcalculating-compass-direction%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
7 Answers
7
active
oldest
votes
7 Answers
7
active
oldest
votes
active
oldest
votes
active
oldest
votes
$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 aDirection
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
add a comment |
$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 aDirection
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
add a comment |
$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$
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;
}
edited May 19 '16 at 15:59
answered Mar 15 '11 at 2:17
munificentmunificent
1,24998
1,24998
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 aDirection
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
add a comment |
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 aDirection
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
9
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
$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
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$
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$
@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
$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
add a comment |
$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
add a comment |
$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
add a comment |
$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$
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().
edited Mar 15 '11 at 4:10
Mahesh Velaga
1033
1033
answered Mar 14 '11 at 20:35
Paulo PintoPaulo Pinto
402210
402210
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
add a comment |
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
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
Mar 16 '11 at 12:33
$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
add a comment |
$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 tostringly-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 classCompassDirectionAngle
isn't to be used outside of the class (it's correctly set to private), I would use aTuple<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 aboutTuples
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
|
show 2 more comments
$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 tostringly-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 classCompassDirectionAngle
isn't to be used outside of the class (it's correctly set to private), I would use aTuple<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 aboutTuples
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
|
show 2 more comments
$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$
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
edited Mar 15 '11 at 16:43
answered Mar 14 '11 at 20:47
SnowbearSnowbear
3,50311423
3,50311423
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 tostringly-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 classCompassDirectionAngle
isn't to be used outside of the class (it's correctly set to private), I would use aTuple<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 aboutTuples
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
|
show 2 more comments
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 tostringly-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 classCompassDirectionAngle
isn't to be used outside of the class (it's correctly set to private), I would use aTuple<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 aboutTuples
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
1
1
$begingroup$
What use is the enum here? Why not simply map numbers to suffixes?
$endgroup$
– pdr
Mar 14 '11 at 21:34
$begingroup$
What use is the enum here? Why not simply map numbers to suffixes?
$endgroup$
– pdr
Mar 14 '11 at 21:34
1
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$
@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$
@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$
+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
$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
|
show 2 more comments
$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
add a comment |
$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
add a comment |
$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$
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.
edited Mar 15 '11 at 2:39
answered Mar 15 '11 at 1:42
pdrpdr
1,661139
1,661139
$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
add a comment |
$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
$begingroup$
I don't really find an extension method appropriate here.
$endgroup$
– Steven Jeuris
Mar 15 '11 at 10:29
$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$
@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$
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$
@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
$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
add a comment |
$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
add a comment |
$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
add a comment |
$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$
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.
answered Aug 9 '12 at 18:16
HightechriderHightechrider
1213
1213
$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
add a comment |
$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
$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
$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
add a comment |
$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$
add a comment |
$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$
add a comment |
$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$
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.
edited Mar 15 '11 at 5:11
answered Mar 15 '11 at 3:58
JustinCJustinC
1114
1114
add a comment |
add a comment |
$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.
New contributor
$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
13 hours ago
add a comment |
$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.
New contributor
$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
13 hours ago
add a comment |
$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.
New contributor
$endgroup$
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.
New contributor
New contributor
answered 14 hours ago
theodore panagostheodore panagos
1
1
New contributor
New contributor
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
13 hours ago
add a comment |
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
13 hours ago
2
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
13 hours ago
$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
13 hours ago
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f1287%2fcalculating-compass-direction%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
$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