Search code examples
c#mathanglepi

Angle Normalization C#


I have an Angle class that has this constructor

public  Angle(int deg, //  Degrees, minutes, seconds
                  int min, //    (Signs should agree
                  int sec) //       for conventional notation.)
{
   /* //Bug degree normalization 
    while (deg <= -180) deg += 360;
    while (deg > Math.PI) deg -= 360;
    //correction end */

    double seconds = sec + 60 * (min + 60 * deg);
    value = seconds * Math.PI / 648000.0;
    normalize(); 
}

and I have these values for testing that constructor

  int[] degrees = { 0, 180, -180, Int32.MinValue / 60, 120+180*200000};
        int[] minutes = { 0, 0, 0, 0,56};
        int[] seconds = { 0, 0, 0, 0,10};
        Console.WriteLine("Testing constructor Angle(int deg, int min)");
        for (int i = 0; i < degrees.Length; i++)
        {

            p = new Angle(degrees[i], minutes[i], seconds[i]);
            Console.WriteLine("p = " + p);
        }
        /*Testing constructor Angle(int deg, int min)
        p = 0°0'0"
        p = 180°0'0"
        p = 180°0'0"
        p = 0°8'0" incorrect output 
        p = -73°11'50"  incorrect output expected  120 56 10 
         */

I do not understand why there is a bug here ? and why did they use divide Int32.MinValue by 60 and 120+180*200000 as this format ?

the comments in the constructor is a correction for the code

UPDATE: Added the code of normalize()

//  For compatibility with the math libraries and other software
//  range is (-pi,pi] not [0,2pi), enforced by the following function:  

void normalize()
{
    double twoPi = Math.PI + Math.PI;          
    while (value <= -Math.PI) value += twoPi;
    while (value >   Math.PI) value -= twoPi;
}  

Solution

  • The problem is in this piece of code:

    double seconds = sec + 60 * (min + 60 * deg);
    

    Although you are storing seconds as a double, the conversion from int to double is taking place after sec + 60 * (min + 60 * deg) is computed as an int.

    The compiler will not choose double arithmetics for you based on the type you decide to store the result in. The compiler will choose the best operator overload based on the types of the operands which in this case are all int and look for a valid implicit conversion (in this case int to double) afterwards; therefore it is choosing int arithmetics and the operation will overflow in the last two test cases:

    Int32.MinValue / 60 * 60 * 60 = Int32.MinValue * 60 < Int32.MinValue which will overflow.

    120 + 180 * 200000 * 60 * 60 > Int32.MaxValue which will also overflow.

    Your expected results for these two cases are probably not considering this behavior.

    In order to solve this issue, change your code to:

    double seconds = sec + 60 * (min + 60f * deg);
    

    Explicitly setting 60 to a double typed literal constant (60f) will force the compiler to resolve all operations to double arithmetics.

    Also, it is worth pointing out that your constructor logic has some other issues:

    1. You should be validating the input data; should it be valid to specify negative minutes or seconds? IMO that doesn't seem reasonable. Only deg should be allowed to have a negative value. You should check for this condition and act accordingly: throw an exception (preferable) or normalize sign of min and sec based on the sign of deg (ugly and potentially confusing).

    2. Your seconds calculation doesn't seem to be correct for negative angles (again, this is tied to the previous issue and whatever sign convention you have decided to implement). Unless the convention is that negative angles must have negative deg, min and sec, the way you are computing seconds is wrong because you are always adding the minutes and seconds terms no matter the sign of deg.

    UPDATE There is one more issue in your code that I missed until I had the chance to test it. Some of your test cases are failing because double doesn't have enough resolution. I think your code needs some major refactoring; normalize() should be called first. This way you will always be managing tightly bounded values that can not cause overflows or precision loss.

    This is the way I would do it:

     public Angle(int deg, int min, int sec)
     {
         //Omitting input values check.
    
         double seconds = sec + 60 * (min + 60 * normalize(deg));
         value = seconds * Math.PI / 648000f;
     }
    
     private int normalize(int deg)
     {
         int normalizedDeg = deg % 360;
    
         if (normalizedDeg <= -180)
             normalizedDeg += 360;
         else if (normalizedDeg > 180)
             normalizedDeg -= 360;
    
         return normalizedDeg;
     }