4

Over the last couple of months I have been diving into coding standard IfSQ. As a part of this IfSQ standard, a rule is to not use Magic Numbers. While I don't have a problem with building this rule into checks as FxCop or StyleCop, I am confused as to what actually IS a Magic Number.

According to Wikipedia there are 3 explanations of magic numbers:

  1. A constant numerical or text value used to identify a file format or protocol;
  2. Distinctive unique values that are unlikely to be mistaken for other meanings (e.g., Globally Unique Identifiers);
  3. Unique values with unexplained meaning or multiple occurrences which could (preferably) be replaced with named constants.

I am focussing on the third meaning. But this is also where I am stuck. Seeing as I am trying to build this in a FxCop rule; the check has to be automated. But what am I supposed flag as magic number?

The IfSQ standard explains the following as magic numbers: Numeric literals (other than 0 or 1) have been embedded directly into the source code. For example "34" or "86400".

But this doesn't sound good to me at all. This would mean I'd have to flag every single occurence of a number other than 0 and 1. Numbers like 24 (hours), 60 (minutes) and 100 (percentage) are, in their correct context, no magic numbers; in my eyes that is.

Because this is will be an automated check, looking from things in a contextual way isn't really possible.

Based on this I have the following questions:

  1. How should I define a magic number?
  2. Is it possible to define magic numbers without context?
  3. Should I skip certain statements when checking for magic numbers?
Matthijs
  • 167

3 Answers3

7

Just to extend a bit on Frank's answer, take the following:

private final int NUMBER_OF_HOURS_PER_DAY = 24;
private final int FRAMES_PER_SECOND = 24;
...
return NUMBER_OF_HOURS_PER_DAY * days;
...
return FRAMES_PER_SECOND * seconds;

Now compare that to:

return 24 * days;
...
return 24 * seconds;

If you consider that one of the most important aspect of code quality is how it conveys intentions, then the later of the above is a disaster.

Community
  • 1
Daniel Dinnyes
  • 530
5

Most tools I know of, which support detection and reporting of magic number usages specifically only address statements. You are quite correct that eliminating all numbers from the source code is kind of pointless.

Who - with a sane mind - would read the number of hours a day has from a configuration file? Of course, there is a good reason to just use the 24, however there is no good reason to use it within a statement that is not part of a constant declaration. Compare the following two occurrences of 24:

private final int NUMBER_OF_HOURS_PER_DAY = 24;
...
return NUMBER_OF_HOURS_PER_DAY * days;

versus:

return 24 * days;

The second example above would typically be flagged as using a magic number, whereas the first one would not. The main difference comes from the fact, that a second usage within the code makes the difference clear: in the first example, you would simply refer to NUMBER_OF_HOURS_PER_DAY again and should your constant change at any time, you are sure that it's updated everywhere at once. In the second example you would just write 24 again, which increases the effect of the magic number problem.

Additionally, the constants can be named appropriately, so you get more meaning from it than from a simple magic number, which may not appear magical to you (because you know that a day has 24 hours), but may appear indeed quite magical to someone else. What if the factor was not 24, but 7? Is that any less magical? and does it even have something to do with the number of days in a week?

Either way, the net effect is that numbers (other than 0 and 1) should be defined via constants and you should not flag those occurrences as magic numbers. Numbers within statements, that are not used at a constant declaration site, should be flagged though.

Border cases: What to do with something like final int HOURS = 20 + 4;? and what about final int CONSTANT = OTHER_CONSTANT * 4 ? Should the 4 be flagged as magic number or not? In my experience, the different tools differ at least in these border cases. Some are very strict and do not allow numbers in expressions that are larger than the constant itself (i.e., no addition/multiplication/etc.). Others are a bit more relaxed and accept it as a usage for defining the constant.

Frank
  • 14,437
2

I've struggled with this definition as well, in my case with CheckStyle.

A particular issue is in the Spring validation framework, where error strings are looked up in a .properties file. CheckStyle complains when I write something like

addError("error.invalid.email.address");

If this is the only place that string is used, I struggle to see what the alternative adds:

private static final String ERROR_INVALID_EMAIL_ADDRESS = "error.invalid.email.address";

.. 200 lines of code here...

addError(ERROR_INVALID_EMAIL_ADDRESS);

Others I'm sure will have differing views...

kiwiron
  • 2,384