Magic numbers, strings and exceptions
-
After finding code like this in our project that was written by a previous developer, i now firmly believe there is a hell. And it will be where you are to maintain code the likes as shown below.
try
{
object.setDistance(objectDistance);
}
catch (IllegalArgumentException exception)
{
if (exception.toString().contains("must be at most"))
throw new IllegalArgumentException("The distance must be at most " +
(int)Math.floor(((12840 * Math.cos(Math.toRadians(45))) + 40) * 2) + ".");
}
PS: This is actual production code and no the method this came from had no comments or documentation.
-
And this is how therapists stay in business... I feel sorry for you, especially trying to understand what all those numbers mean.
-
It looks to me that he is trying to calculate the maximum distance between any two points on earth.
12840 is pretty close to the diameter of the earth - 12715km.
Although using Math.floor would give a value slightly below the maximum, the overestimated diameter would compensate.
-
Instead of magic numbers the author should obviously have used an application setting to make sure that the application can easily be customized to be used on another planets and celestial objects.
-
@Spikeles said:
After finding code like this in our project that was written by a previous developer, i now firmly believe there is a hell. And it will be where you are to maintain code the likes as shown below.
try
{
object.setDistance(objectDistance);
}
catch (IllegalArgumentException exception)
{
if (exception.toString().contains("must be at most"))
throw new IllegalArgumentException("The distance must be at most " +
(int)Math.floor(((12840 * Math.cos(Math.toRadians(45))) + 40) * 2) + ".");
}
PS: This is actual production code and no the method this came from had no comments or documentation.Ok I'm confused. Why the F*** does he perform the calculation every time? This is one of those cases where a "magic number" should be used. Set a variable at the top or set a constant, comment it and use it here instead of the equation. It is, after all, an approximation so using a set value becomes even less of a problem, not to mention the speed increase over this.
-
There are no variables in the statement, as well there shouldn't be unless it's being ported to a different planet. Instead of all the math in the function, why not just put the actual max distance in the string and save a bit of time and effort? (Max distance on Earth = c. 12,000 miles.)
-
@Gsquared said:
There are no variables in the statement, as well there shouldn't be unless it's being ported to a different planet. Instead of all the math in the function, why not just put the actual max distance in the string and save a bit of time and effort? (Max distance on Earth = c. 12,000 miles.)
Funny thing is, I guess that the original exception already shows that...
-
@mallard said:
It looks to me that he is trying to calculate the maximum distance between any two points on earth.
12840 is pretty close to the diameter of the earth - 12715km.
Although using Math.floor would give a value slightly below the maximum, the overestimated diameter would compensate.
Actually, the common value for the mean radius of the earth is 6,370,100 m; so the diameter is closer to 12740 km, which looks like he just typed incorrectly. I've never seen the 12715km number before - is that polar diameter? (equatorial is a bit larger). Back-of-the-envelope calculations are to just use 6400 km.
Ah, I have confirmed that the 12715km is polar diameter: see here; also the equatorial is 12756km and some change. Incidentally, the polar number is probably worse than equatorial for use as it has the larger error from the mean.
-
I like how nobody has even mentioned that he's testing the exception by comparing the string the exception contains against an English string. Or how he could have just checked the distance and thrown if it was not correct, rather than calling the setDistance () method, waiting for it to throw, catching it, incorrectly testing the exception against an English string, creating a complicated string from magic numbers, and throwing that (which also involves allocating memory in this case). The whole thing is just really lovely.
-
That reminds me of some terrible mispractice in Javascript though, to throw human-readable strings as "exceptions"... some people didn't understand the concept of exceptions very well...
-
Just to clarify a point. 12840 has nothing to do with the diameter of the earth. Although that is a pretty funky coincidence. I'm pretty sure it's some random upper bound in metres the dev put in because the boss said so.
And i would guess the conversation went like..
"Programmer: So what is the highest limit this can be?"
"Boss: I dunno. Does 13 metres sound good?"
"Programmer: Yeah ok, but just to be on the safe size i'll take it down a bit"
This kind of hard coding, undocumented crap is scattered throughout the source. And i have thought on many occasions to simply wipe it and restart from scratch. I can show you more if you really want to have your eyes bleed.
