5
\$\begingroup\$

This program uses the C language. What's used are two points, and the usage of the inverse tan function finds the angle of the line between the two points. Afterward, it finds the cardinal direction on the plane. -y is south, +y is north. It seems to work quite well. However, I want to find what is unconventional of my program. I want to learn to write good code and this is a good step towards it.

#include <stdio.h>
#include <stddef.h>
#include <stdlib.h>
#include <math.h>

#define PI 3.14159265

int cardinal(double degrees)
{
    // 0 is North
    // 1 is North-east
    // 2 is East
    // 3 is South-east
    // 4 is South
    // 5 is South-west
    // 6 is West
    // 7 is North-west
    int cardinal;
    if (degrees >= -22.5 && degrees <= 22.5)
        cardinal = 0;
    if (degrees > 22.5 && degrees < 67.5)
        cardinal = 1;
    if (degrees >= 67.5 && degrees <= 112.5)
        cardinal = 2;
    if (degrees > 112.5 && degrees < 157.5)
        cardinal = 3;
    if (degrees >= -157.5 && degrees <= 157.5)
        cardinal = 4;
    if (degrees > -157.5 && degrees < -112.5)
        cardinal = 5;
    if (degrees >= -112.5 && degrees <= -67.5)
        cardinal = 6;
    if (degrees > -67.5 && degrees < -22.5)
        cardinal = 7;

    return cardinal;
}

char* direction(int cardinal)
{
    if (cardinal == 0)
        return "north";
    if (cardinal == 1)
        return "northeast";
    if (cardinal == 2)
        return "east";
    if (cardinal == 3)
        return "southeast";
    if (cardinal == 4)
        return "south";
    if (cardinal == 5)
        return "southwest";
    if (cardinal == 6)
        return "west";
    if (cardinal == 7)
        return "northwest";
}

int main()
{
    int xpl = 494, ypl = 105;
    int xcom = 152, ycom = -168;

    double degrees;
    double radians;

    int num;

    double CONS = 180.0 / PI;

    int xo = xcom - xpl;
    int yo = ycom - ypl;

    radians = atan2(yo, xo);
    degrees = radians * CONS;

    num = cardinal(degrees);

    printf("The angle to travel in is %f (degrees) %f (radians)\n", degrees, radians);
    printf("Numerical direction: %d", num);
    printf("\nDirection: %s", direction(num));

    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Why don't you use switch/case in direction function? And enum instead of "magic" numbers? \$\endgroup\$ – Anton Glukhov Apr 12 '17 at 5:29
  • \$\begingroup\$ I understand that I could have used switch/case in direction function, but what do you mean by the use of enums and "magic numbers"? \$\endgroup\$ – Tristen Apr 12 '17 at 5:41
3
\$\begingroup\$

Two suggestions:

  1. in the cardinal function, return the number rather than setting a variable, so you shortcut all the other if statements. You could further optimize that by doing a binary rather than a linear search.
  2. your direction function could be replaced by an array: int directions[8] = {"north", "northeast", ... };. Then the lookup is simply directions[cardinal]
\$\endgroup\$
  • \$\begingroup\$ Thank you. A binary search is actually quite clever in how I am to find the cardinal numbers. \$\endgroup\$ – Tristen Apr 12 '17 at 5:43
  • \$\begingroup\$ Actually, since you are dividing into equal pieces, I like @holroy's suggestion of calculating the cardinal number better than a binary search. \$\endgroup\$ – gens Apr 12 '17 at 20:22
  • \$\begingroup\$ Note OP apparently is not giving a symmetric conversion to N as NE, etc. N,E,S,W win the tie breakers at both edges. This adds a difficulty to an algorithmic computation not taken into account with @holroy's suggestion \$\endgroup\$ – chux Apr 13 '17 at 1:17
3
\$\begingroup\$

Your implementation misses a few things when coming to good coding practices, but let us first restate your goal: Calculate a degree based on line coordinates. Return a cardinal number evenly dividing the circle, and/or return a text related to the cardinal number.

Code review

  • OK structure – The structure of your program is OK, and it easy enough to read.
  • Enclose blocks in braces – I'm not a fan of not enclosing the if block in braces. Your indentation is good, and they are single statements so it is legal, but still this will lead to an error somewhere along the road. Please enclose if blocks in braces.
  • Degree calculation not a function? – Why haven't you made the actual calculation into a function? Wouldn't it be natural to have a double line2degree(x1, y1, x2, y2) function in your program?
  • Use correct return types – Your direction() function returns char *, which I read as a char pointer to something I can change. What will happen if you start changing it? Use const char * to properly announce this is a constant string.
  • "Wrong" declaration of main() - Please use a more standard version for declaring your main function, like int main(int argc, char** argv). Even though you don't use it right now, it's better practice to do this.
  • Strip down main() to the bare minimum – In my opinion it should only contain some argument checks (if needed), and then call of other functions. This will make it easier later on to extend your program either as a standalone program, or as part of a library, and so on.
  • No error checking – You don't check that the degrees are within the expected range of -180 to 180. You don't check that cardinal is between 0 and 7. Both cases leading to unexpected results, or no results.
  • Use of magic numbers – In general to sprinkle bare constants all over your code is not considered good practices. This both applies to the degrees in cardinal(), and the actual cardinal number (0-7). What if you want to change it to another range? Like 0-15? Or 0-9?. In general it would be better to declare these as constants, like you did for PI, and use the constants in your program. Or find a way to avoid using the specific numbers overall.
  • Extensive use of multiple if statements – Whenever you find yourself writing more than a few if statements back to back, you need to reconsider whether there are better ways of tackling the issue.

Code refactor

Here are some ideas for refactoring your code.

  • Add line2degree() – This seems like a natural extension to your code
  • Do error checking! – Either normalize the input values to be within your expected range, or do some error handling when input values are outside of the range
  • Calculate the cardinal number – What would happen if you did (degrees + 180.0 + 22.5)/ 45? You can rather easily calculate the cardinal number by recognising that you've divided the circle into even 45 degree parts. This would also restrain the need of constants. Now you could have DEGREE_OFFSET = 202.5 and DIVIDING_ANGLE = 45. Probably you could do the calculation in int's also.
  • Use an array for the direction texts – As suggested in another post, you could easily return the cardinal text from an array (after checking array boundaries).
  • New feature: Extend the main to allow for entering new inputs – If you look into it, you could rather easily allow for the new coordinates to be entered on the command line, making it easier to test multiple conditions.

I don't have time to code this, but I hope you get the gist of it, and come back with another version in a little while!

\$\endgroup\$
  • \$\begingroup\$ Wow, thank you very much! I knew there was a way to calculate the cardinal number by an expression, but I didn't know how to do it albeit I did it on paper! \$\endgroup\$ – Tristen Apr 12 '17 at 13:39
1
\$\begingroup\$

Bug #1

Your program doesn't give the correct answer because atan2() returns 0 to mean "east" but your program interprets 0 to mean "north". You need to adjust your cardinal() function to match the meaning of atan2()'s return value..

Bug #2

Your cardinal() function has an additional bug in that it doesn't handle angles greater than 157.5 or less than -157.5 correctly. In other words, your function will never return "south" (which actually would be "west" after you fix the first bug). Your logic for that case is actually reverse of what it should be.

\$\endgroup\$
  • \$\begingroup\$ On my review of atan2(() I now see my suggested commnet was indeed in error. Comment deleted. \$\endgroup\$ – chux Apr 13 '17 at 1:15
1
\$\begingroup\$

Code needs more robust testing. Some mods to create easier testing are below. With such testing, the functional error would be readily noticed. Not so surprisingly, test code can be harder and longer to write than the code-under-test.

void mainxy(int xo, int yo) {
  double CONS = 180.0 / PI;
  double radians = atan2(yo, xo);
  double degrees = radians * CONS;
  int num = cardinal(degrees);

  printf("Angle is %11f (degrees) %9f (radians)  ", degrees, radians);
  printf("Numerical dir.: %2d  ", num);
  printf("Dir.: %s\n", direction(num));
}

int main() {
  for (double degrees = -360.0; degrees < 361.0; degrees += 360.0 / 8) {
    mainxy(100 * cos(degrees * PI / 360), 100 * sin(degrees * PI / 360));
  }
}

Output

Angle is  180.000000 (degrees)  3.141593 (radians)  Numerical dir.:  0  Dir.: north
Angle is -157.557247 (degrees) -2.749893 (radians)  Numerical dir.:  0  Dir.: north
Angle is -135.000000 (degrees) -2.356194 (radians)  Numerical dir.:  5  Dir.: southwest
Angle is -112.442753 (degrees) -1.962496 (radians)  Numerical dir.:  6  Dir.: west
Angle is  -90.000000 (degrees) -1.570796 (radians)  Numerical dir.:  6  Dir.: west
Angle is  -67.557247 (degrees) -1.179096 (radians)  Numerical dir.:  6  Dir.: west
Angle is  -45.000000 (degrees) -0.785398 (radians)  Numerical dir.:  7  Dir.: northwest
Angle is  -22.442753 (degrees) -0.391700 (radians)  Numerical dir.:  4  Dir.: south
Angle is    0.000000 (degrees)  0.000000 (radians)  Numerical dir.:  4  Dir.: south
Angle is   22.442753 (degrees)  0.391700 (radians)  Numerical dir.:  4  Dir.: south
Angle is   45.000000 (degrees)  0.785398 (radians)  Numerical dir.:  4  Dir.: south
Angle is   67.557247 (degrees)  1.179096 (radians)  Numerical dir.:  4  Dir.: south
Angle is   90.000000 (degrees)  1.570796 (radians)  Numerical dir.:  4  Dir.: south
Angle is  112.442753 (degrees)  1.962496 (radians)  Numerical dir.:  4  Dir.: south
Angle is  135.000000 (degrees)  2.356194 (radians)  Numerical dir.:  4  Dir.: south
Angle is  157.557247 (degrees)  2.749893 (radians)  Numerical dir.:  0  Dir.: north
Angle is  180.000000 (degrees)  3.141593 (radians)  Numerical dir.:  0  Dir.: north

No need to code PI to some approximation that might or might not be the best for an implementation when a one-time calculation can solve that.

// double CONS = 180.0 / PI;
double CONS = 180.0/acos(-1.0);  
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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