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?
  • 2
    Why are 24, 60 and 100 not magical to you ? Just because you know what they mean ? That criterion could be uses to ANY number since someone knows what any given number means. Stick to the canonical definition and don't make your own. – Tulains Córdova Jul 29 '14 at 10:07
  • @user61852: You are correct on that. But how would I distinguish non-magic numbers and magic numbers then? Seeing as any number could mean something to someone. If you were a computer, what would you describe as a magic number? From a purely objective point of view? – Matthijs Jul 29 '14 at 10:09
  • Easy: any literal number other than 1 and 0 which is not part of a constant declaration. – Tulains Córdova Jul 29 '14 at 10:11
  • But what about this: constant int x = 25 + 5? You are declaring a constant, but with the use of two numbers. And how about this: constant int y = x + 5. – Matthijs Jul 29 '14 at 10:12
  • 1
    Those are not magic numbers according to the definition, because those are constant declarations. If you get fastidios about it you will need to provide your script with an artificial intelligence similar to that of C3PO or Skynet in order to know every context of every human or alien culture. Is that the scope of your work ? Such a script will eventually become self-aware and revel agains humans. Please don't do that. – Tulains Córdova Jul 29 '14 at 10:23
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.

  • The way I have my technology now, is that with hours = 20 + 4; both 20 and 4 are flagged as magic numbers. Looking at this from a contextual point of view, I can see these should not be flagged. But there is my problem again of not really being able to look at it from that point of view. What would you recommend from a non-contextual point of view? Should I skip over all constant-assignments? – Matthijs Jul 29 '14 at 9:30
  • If your standard is clear, then you have to adhere to it. Another constant defined as foo = 20 * 2 may or may not be a multiple occurrence of the 20. If unsure, better report a false positive, than missing something. At least that's the rule of thumb for verification tools. – Frank Jul 29 '14 at 9:34
  • While the standard is clear, I do not agree with the standard on this point. I guess rules are there to be broken, in this case? The problem with this is that, if I let through too many false-positives, the amount of false-positives on 1000+ lines of code would really get out of hand. I am looking for a line to draw as to when to flag it or not. The standard has drawn a line way too high (or low, depending on your viewpoint), for me. – Matthijs Jul 29 '14 at 9:39
  • Make it a configurable behavior then. Your tool is not free to ignore the standard at this point, just because you do not agree. Others may have to depend on the tool to be 100% accurate to what the standard requires, no matter if they agree with that standard or not! – Frank Jul 29 '14 at 11:04
  • There might be a small misunderstanding here, I am not developing a commercial tool of some sort. We have decided to include the IfSQ-standard in our companies standards; but with a few changes here and there. We have done the magic-numberdiscussion ourselves but couldn't come to one solid conclusion, hence the question here. The FxCop-rule (a code analysismechanism) will only be used internally. – Matthijs Jul 29 '14 at 11:08
5

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.

  • 1
    @Vatine My main point is that in programming numbers are used without units of measurements. One important aspect of physics is the calculus of quantities, which tells you for example that hours/day * days = hours, and that frames/second * seconds = frames. This aspect is almost always left out of arithmetic in code, and only conveyed through variable names. The intention above is that one 24 is not equal to the other, because they have different units, though implicit. – Daniel Dinnyes Jul 29 '14 at 14:57
  • 1
    @DaniellDinnyes Exactly. It would have been much easier to read if I'd used "HoursPerWeek" instead of 168 (which is the right number, but not a constant most people recognize on sight). – Vatine Jul 29 '14 at 15:59
  • 1
    Either they will be invaders from earth, in which case I must not chuck UTC (earth standard time...), however much a misfit it might be, or the sub-divisions we make would probably be useless, so I would have to do a complete re-write anyway. Just as serious as you here ;-) – Deduplicator Dec 18 '14 at 10:09
  • 2
    @Deduplicator, I think you missed the point of Daniel's answer. It's obvious that there are 24 "hours per day", but it's not obvious if that's the concept you are referring to when there are other "24" constants floating around in your code. – UncleZeiv Dec 18 '14 at 10:43
  • 1
    @Deduplicator, sure, but while I do think that yours is a valid point, it is more an answer to the general "raw number vs. named constant" debate, it doesn't seem to address the concern that Daniel raised here. hours_per_week = 24 * 7 is fine, but frames_per_duration = 24 * 60 * 60 is already a bit more confusing... are we computing seconds per day or frames per hour? Sure, you might argue that the variable is named incorrectly, still naming the constants would help as well. – UncleZeiv Dec 19 '14 at 16:27
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...

  • 1
    The value is most obvious on the second usage. Consider someone wanting to do something like hasError("error.invalid.email.address"). Not DRY at all. – Frank Jul 29 '14 at 9:29
  • It may be the only place that string is used now. How can you know that will always be the case? If you keep CheckStyle happy, all of your error strings will be in the properties file. Anybody adding another report of the same error will find the existing error constant. And anybody just looking at the properties file will see a pretty comprehensive list of all the errors the code checks for. – itsbruce Jul 29 '14 at 9:30
  • My beef is that the connection between the line of code and the properties file appears to be broken by defining a totally spurious string constant. Are you guys suggesting that someone wanting to re-use this property string is going to be helped by the spurious indirection? I would expect that they would look at the property definition, then hard-code the key. – kiwiron Jul 29 '14 at 9:37
  • I wonder if there is already a tool for generating the Java constants source file given the ".property" file as input. For example, the generated constant file could contain public final class Strings { public static final class Error { public static final class Email { public static final String Address = "error.invalid.email.address"; } } } – rwong Jul 29 '14 at 10:17
  • In other platforms, such as Windows and Android, such property files and constant-value files belong to the collection of "resource files" consumed and generated (updated) by the build system. – rwong Jul 29 '14 at 10:19

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.