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
  • 1
  • 4
  • 3
    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
  • According to the definition they are not magic numbers, but contextually, what do they mean? Why, in this case, 25 and 5? Even if I'd give a meaningful name to the constant the question still rises why exactly those numbers: `constant int Amount_Of_Players = 4 + 4`. – Matthijs Jul 29 '14 at 10:32
  • Again, providing your script with the intelligence to discern human culture contexts exceeds the scope of your project. Stick with the definition and save yourself decades of work and millions of dollars in R&D. – Tulains Córdova Jul 29 '14 at 10:35
  • [Clarke's third law](https://en.wikipedia.org/wiki/Clarke%27s_three_laws) says that _"Any sufficiently advanced technology is indistinguishable from magic."_ ([see here](http://www.cs.utah.edu/~elb/folklore/magic.html)) If you can't explain it, then it's magic. Simple. – Daniel Dinnyes Jul 29 '14 at 15:12
  • Possible duplicate of [Eliminating Magic Numbers: When is it time to say "No"?](http://softwareengineering.stackexchange.com/questions/56375/eliminating-magic-numbers-when-is-it-time-to-say-no) – gnat Apr 08 '17 at 12:57
  • Possible duplicate of [Is every number in the code considered a "magic number"?](https://softwareengineering.stackexchange.com/questions/181921/is-every-number-in-the-code-considered-a-magic-number) – gnat Sep 11 '18 at 06:28

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
  • 520
  • 4
  • 10
  • Not to mention `return 168 * weeks`... – Vatine Jul 29 '14 at 12:24
  • 1
    @Vatine My main point is that in programming numbers are used without [units of measurements](https://en.wikipedia.org/wiki/Units_of_measurement). One important aspect of physics is [the calculus of quantities](https://en.wikipedia.org/wiki/Quantity_calculus), 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
  • @Vatine When I was a child there was a weekly newspaper in my country called [168 hours](https://hu.wikipedia.org/wiki/168_%C3%B3ra), so I recognized it ;) – Daniel Dinnyes Jul 29 '14 at 18:38
  • For 24 hours per day, using the raw number is appropriate. Everyone reading it (who has a chance in hell of understanding anything either way) will understand, and appreciate *not* having to check the pointless constant for correctness too. For your second example with the frame-rate, yes, that's a magic constant which should be an appropriately named constant. – Deduplicator Dec 18 '14 at 01:50
  • @Deduplicator Which planet are we talking about, 24? Martians might understand it as well, but the universe is large... bit more humbleness there, please. One day you might want to sell your software in Alpha Centauri. Just change a single constant and it's ready to ship, how about that? ;) – Daniel Dinnyes Dec 18 '14 at 10:06
  • 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
  • @Deduplicator That's a [familiar story](http://www.joelonsoftware.com/articles/fog0000000069.html). There are [better options](http://www.paulgraham.com/avg.html) than that, which will make your solution [concise and maintainable](http://steve-yegge.blogspot.com/2007/12/codes-worst-enemy.html) ;) – Daniel Dinnyes Dec 18 '14 at 10:46
  • @UncleZeiv Indeed, that was the point about "conveying intentions". My other point though was that one day you might need to update that "constant" (it should be [23.934 hours](https://solarsystem.nasa.gov/planets/profile.cfm?Display=Facts&Object=Earth), according to NASA). Then you have to edit all the places with `24` in your code base. Combine that with `24` being used with multiple meanings, plus the practice of hiring new contractors every 6 month to continue from where the previous left off, and things will probably [work out just fine](http://www.bbc.com/news/business-30125728). – Daniel Dinnyes Dec 18 '14 at 11:10
  • @UncleZeiv: Nope, you don't seem to get the point of my comment: Using named constants instead of raw literals has a cost of its own, namely having to check proper semantic use of the constant and having to check proper definition of it. Instantly recognizable raw literals have a lower overhead on reading than some_constant_with_a_long_name which might be still ambiguous, even though it is far too long for comfort. – Deduplicator Dec 18 '14 at 11:14
  • @DanielDinnyes: And regarding changing some hours_per_day constant, that would probably break lots of your code anyway, as it was an integer before. BTW: I don't see how [those things you linked here](http://programmers.stackexchange.com/questions/251540/when-is-a-number-a-magic-number/251545?noredirect=1#comment542979_251545) are relevant? – Deduplicator Dec 18 '14 at 11:15
  • @Deduplicator not strictly relevant you are right. They converge around the subject of how code maintainability can make or break a business, which I thought is related to this discussion. – Daniel Dinnyes Dec 18 '14 at 12:10
  • Well, it's related in that the question is about magic numbers, and when they aren't really magic, and thus about writing clear and maintainable code. – Deduplicator Dec 18 '14 at 12:16
  • 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
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,407
  • 3
  • 41
  • 66
  • 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 09: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 09: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 09: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
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,278
  • 10
  • 13
  • 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 09: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 09: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 09: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
  • Java does not do this by default, so @kiwiron is correct that the indirectional copy is a problem. There are however approaches based on the Java Annotation Processing framework to deal with this. See for example Netbeans' `NbBundle.Messages` annotation. – Frank Jul 29 '14 at 11:18