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;
}
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:
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).
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;
}