11

This question already has an answer here:

I've been reading many of the articles and such about magic numbers, i.e.

  1. wikipedia article
  2. programmers.stackexchange.com article

It seemed like no one has considered context as a means of making it acceptable to use a number and not have it considered magic.

For example

connection.setLoginTimeoutSeconds(30);

Vs

final int LOGIN_TIMEOUT_30_SECONDS = 30;
connection.setLoginTimeoutSeconds(LOGIN_TIMEOUT_30_SECONDS);

or even

final int LOGIN_TIMEOUT = 30;
connection.setLoginTimeoutSeconds(LOGIN_TIMEOUT);

The function name and value has all of the information as the variable name and value:

setLoginTimeoutSeconds(30);
LOGIN_TIMEOUT_SECONDS = 30;

In the case where a variable for the login time out is not needed anywhere else in the code it seems like the first example is more readable and even more maintainable than the latter two and that the number 30 is not magic because of where it resides. Does anyone agree or disagree?

I am not asking about well known constants here like in are-all-magic-numbers-created-the-same. Any constant value can will do. Nor am I asking for my example to be found as flawed because someone could make a problem from it. What I am really asking is form anyone's opinion on if magic number rules need not apply.

marked as duplicate by whatsisname, GlenH7, user22815, Ross Patterson, durron597 Oct 9 '15 at 4:33

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 23
    That's a really bad example. A timeout value is particularly likely to be subject to requirements changes. And it will almost certainly be easier to find that value if it's in a list of application parameters, and not buried five levels deep in the business code. – Kilian Foth Oct 8 '15 at 15:11
  • 3
    its not a 'magic number' because its doesnt have a secret meaning – Ewan Oct 8 '15 at 15:15
  • 17
    I would never name a constant LOGIN_TIMEOUT_30_SECONDS. The name should never contain the value, rather, it should simply serve to describe what the constant is for. – rory.ap Oct 8 '15 at 15:44
  • 5
    And the name should be as specific as possible, e.g. LOGIN_TIMEOUT is also bad and LOGIN_TIMEOUT_SECONDS would be better. – rory.ap Oct 8 '15 at 15:46
  • 3
    @roryap The unit is type information, so if you have a sufficiently powerful type system that should really be part of the type (e.g. LOGIN_TIMEOUT = new Seconds(30);) and not the name. Similarly for the function name if you want to be pedantic, which should take an abstract duration type rather than just seconds. It "makes the code more readable and flexible" and "increases reusability". – Thomas Oct 8 '15 at 19:09
6
final int LOGIN_TIMEOUT_SECONDS = 30;
// other code
connection.setLoginTimeoutSeconds(LOGIN_TIMEOUT_SECONDS);

This seems to solve all the problems/dilemma you are wondering about.

It also helps with some quick verification, since you see you are passing correct units into the method (if it was setLoginTimeoutMS(LOGIN_TIMEOUT_SECONDS) you'd clearly see an error.

I was going to write a comment but want instead to address:

It seemed like no one considered has context as a means of making it acceptable to use a number and not have it considered magic.

Context changes. You don't want to rely on something which may change.

Let's look at your "good" example:

final int LOGIN_TIMEOUT = 30;
connection.setLoginTimeoutSeconds(LOGIN_TIMEOUT);

The problem with magic numbers is not knowing what they mean. In this example your naming doesn't really help and makes it worse. Is 30 in seconds? Was there a unit conversion problem?

I know that your connection takes a value in seconds, but the problem is that your magic constant doesn't help make this clear. It can be really easily read "call the login timeout function, which takes an argument of seconds, and pass in a value of arbitrary time which hopefully is seconds."

If you declare this immediately before the login call it can be clear. But what happens when the code is refactored to have a common login timeout? Or when other code gets added between the definition/usage?

And last, don't depend on the context to add clarity when you can make very simple naming changes to add the same clarity. Context changes. If someone moves that declaration to a header file to support common usage, your context is gone. Boom. And just like that you have made your system less maintainable.

Also note there are times when a constant may be beneficial (single usages, etc). But the question you are asking still applies - you just may know more matter-of-fact that the context won't change.

  • 1
    What's this? Apps hungarian? Don't mind if I do! – Nathan Tuggy Oct 8 '15 at 18:09
  • 2
    Really, your setLoginTimeout should be taking some sort of Duration, not a integer. Much harder to mess up. – Paul Draper Oct 8 '15 at 23:18
  • @PaulDraper Agreed, something like connection.setLoginTimeout(30, TimeUnit.SECONDS); or TimeSpan timeoutPeriod = new TimeSpan(30, TimeUnit.SECONDS); connection.setLoginTimeout(timeoutPeriod); – Zymus Oct 9 '15 at 3:45
  • @Zymus, and preferably the last one. You don't have to double all your parameters, and you can use it for a return value. – Paul Draper Oct 9 '15 at 3:56
22

It isn't just that the number itself is bad, but that misunderstandings can be introduced when the purpose isn't made clear and there a number of such values. Building on your example:

connection.setLoginTimeout(30); 
connection.setSessionTimeout(30);

Unless you're intimately familiar with the code, it isn't clear whether both values should be the same or whether they can be different. So if they are supposed to be the same then:

const TIMEOUT = 30;

connection.setLoginTimeout(TIMEOUT); 
connection.setSessionTimeout(TIMEOUT);

Or if they could be different values then:

const LOG_TIMEOUT = 30;
const SESS_TIMEOUT = 30;

connection.setLoginTimeout(LOG_TIMEOUT); 
connection.setSessionTimeout(SESS_TIMEOUT);

You may argue that this doesn't apply for one setting, but should another be added later with the same initial value, the misunderstanding I've outlined above could easily occur.

  • So it is not a magic number but a bad practice for other reasons then. – superbAfterSemperPhi Oct 8 '15 at 15:43
  • 2
    In the wikipedia article you cite, such numbers are called: Unnamed numerical constants and that is exactly what they are. From the article: The use of unnamed magic numbers in code obscures the developers' intent in choosing that number – Robbie Dee Oct 8 '15 at 15:50
  • 1
    @RobbieDee -- Seconds, minutes, whatever. The point is, if you don't include that, the names are too vague and not as useful. – rory.ap Oct 8 '15 at 15:51
  • 1
    @roryap Good point - it is perfectly possible that setLoginTimeout could be in seconds and setSessionTimeout could be in minutes. – Robbie Dee Oct 8 '15 at 15:56
  • 3
    This good answer would be great if you pointed out that the session timeout is likely 30 minutes and that if someone did a find/replace, bugs would ensue. – RubberDuck Oct 8 '15 at 16:53
10

The first major problem with

connection.setLoginTimeout(30);

Is that you're assuming the junior developer you just hired who's looking at this 6 months from now will know that "30" is seconds & not milliseconds.

The second major problem (as @KilianFoth pointed out in a comment) is that again, 6 months from now, that junior developer might not be able to find this parameter very easily if at all.

An important point not brought up in your examples or the examples in the other question is that there is absolutely no reason to declare the constant right next to where it's used.

Instead, stick all of those "tuning parameters" in a common well documented place - perhaps a Constants.h file or equivalent.

  • So it is not a magic number but a bad practice for other reasons then. – superbAfterSemperPhi Oct 8 '15 at 15:43
  • Yes, it is still a magic number because it's a numeric literal, not a named constant. – Simon B Oct 8 '15 at 16:23
  • 1
    @SimonB: numeric literals aren't necessarily magic numbers. There are situations where numeric literals are the best option. – whatsisname Oct 8 '15 at 16:29
  • Is it a seconds or milliseconds should be visible from the signature of the function - this is not caller's responsibility – StupidOne Oct 8 '15 at 20:34
  • If the junior developer you just hired is incapable of quickly looking up the documentation for setLoginTimeout, you shouldn't have hired him. Thank goodness he's still in his probationary period! Or if you didn't write any documentation then, well, you shouldn't have hired yourself I guess... – Lightness Races in Orbit Oct 8 '15 at 21:00
3

setLoginTimeoutSeconds(30) tells you that 30 is (now) the timeout.

It provides no explanation about why we set the timeout to 30.
Is it because the server timeout is 20 and we want an additional 10 seconds for network delays? Is it the same with the logout timeout that's defined in the next line? Is it something that was decided by usability testing and sits in a configuration file? Or maybe it has no meaning and was just picked as a default randomly (something that is still good to know)?

Yes, it could be that there is nothing more to say about this number in which case naming it TIMEOUT is equally meaningless with calling it THIRTY (and TIMEOUT_IN_SECONDS isn't particularly helpful either; this should be ideally deduced by the context imho). But in that case, why do we need to set it at all? It might make more sense to have the connection work without having to set it to 30.

In other words, if there's nothing outside the context to be said, perhaps there shouldn't be a number at all.

1

The primary purpose of the DATA statement is to give names to constants; instead of referring to pi as 3.141592653589793 at every appearance, the variable PI can be given that value with a DATA statement and used instead of the longer form of the constant. This also simplifies modifying the program, should the value of pi change.

Xerox Basic FORTRAN and Basic FORTRAN IV Manual, attributed to David H. Owens. Source: https://en.wikiquote.org/wiki/Fortran


Of course, commonly used constant values should be used via constant declarations which make it plainly evident to whoever is looking at the code what the value stands for, and to make it easy to change the value in the future, if need be. However, this is not to be viewed as some absolutely binding dictum that should be followed with dogmatic devotion.

Consider the following implementation of a hashCode() function:

/**
 * Computes the FNV hash code of all elements in this unmodifiable enumerable.
 */
static <E> int hashCode( UnmodifiableEnumerable<E> self )
{
    int hash = 17;
    for( E element : self )
        hash = 31 * hash + Objects.hashCode( element );
    return hash;
}

As the comment says, this is the (most common simplified version of the) standard Fowler–Noll–Vo hash function that most programmers are familiar with. There is no point in creating constants for 17 and 31, since that would only further complicate things. They are just two prime numbers, their use is highly localized, (only within this function,) they are highly unlikely to ever change, and if there is ever a need to change them, the person who will decide to change them better know very well what they are doing, and therefore better know very well what they stand for.

Other times, there exist numbers that are so permanently cast in stone, and so very well known by everyone, that constants are sometimes not necessary. The hilarious quote from the Basic FORTRAN IV Manual illustrates this nicely, but here is another example: A week has seven days everywhere around the world, and this is very highly unlikely to ever change, so a simple expression which involves existing identifiers that already have meaningful names does not have anything to benefit from replacing a hard-coded literal 7 with a constant. Who really needs to see days = weeks * DAYS_PER_WEEK instead of days = weeks * 7?

(Of course, if the expression is more complicated, or if the other identifiers that take part in the expression do not have such self-explanatory names, it may still be better to use DAYS_PER_WEEK rather than 7, for the purpose of documenting what you are doing.)

That having been said, you have to be careful with cultural issues.

On page 300 of the book "Clean Code" by Prentice Hall, author Robert C. Martin says:

And in the FEET_PER_MILE case, the number 5280 is so very well known and so unique a constant that readers would recognize it even if it stood alone on a page with no context surrounding it.

To which I have to say (excerpt from my blog)

The world is not the USA. Most of the world uses the metric system, and knows very little about miles and feet. When given a number in "United States customary units", many people outside the USA know how to convert it to metric, but it is rare to meet someone who is proficient in converting from metric to US units, and even more rare to meet someone who has the slightest clue, or the slightest interest, in converting between US units. Therefore, the number 5280 means absolutely nothing to the vast majority of the population of the planet. As a matter of fact, there are so many non-USAians working as software engineers in the USA, that even in the USA it would be a bad idea to assume that anyone who sees the number 5280 in a piece of code will know what it stands for.

Finally, magic numbers in their true sense (of having some secret meaning) should nearly always be declared with a constant, otherwise their occurrence in the middle of source code can be very puzzling, but even this rule has an exception. If you are only going to use a particular magic number once in your entire code base, and if it is to appear in a piece of code like the following, then it is okay, because what is going on is very obvious:

if( fileObject.getMagicNumber() == 0xbadbeef ) { ... }
  • 1
    Re: "For example, a week has seven days everywhere around the world, so there is no point in having a DAYS_PER_WEEK constant": That doesn't necessarily follow. Using 'DAYS_PER_WEEK' instead of '7' doesn't just tell you that 7 is the number of days in a week, it also tells you that this is the relevant fact about 7. (This is especially an issue if there are other vaguenesses in the surrounding code. event.postpone(2 * 7) is inscrutable; event.postpone(2 * DAYS_PER_WEEK) is infinitely better.) – ruakh Oct 8 '15 at 23:35
  • @ruakh Yes, you are right, there are cases where things might be improved by using constants even for such things as DAYS_PER_WEEK and BITS_PER_BYTE. And when other hard-coded numbers are also involved in the expressions, that definitely tends to be one of those cases. – Mike Nakis Oct 9 '15 at 5:09
  • @ruakh someone upvoted my answer, so I remembered it 2 years later. I fixed the problematic sentence. – Mike Nakis Feb 2 '17 at 8:19
0

Another point not yet mentioned is that there are times when a value ends up getting encoded within the logic of a program in addition to appearing as a literal. For example, a function to compute the average of three consecutive values in an array may be written more efficiently and cleanly as

Average = (arr[i-1]+arr[i]+arr[i+1])/3.0; // assume arr is double[]

than using a loop (doing five without a loop would be pushing things, and seven would probably be cleaner with a loop, but for three things the in-line code is simpler). The 3.0 literal semantically repeats the number of terms being added, which is a slight violation of "Don't Repeat Yourself", but since both usages are close together it will be pretty obvious how they relate. Making the 3.0 a named constant would make it far less clear that changing the constant would not cause a greater number of terms to be correctly averaged together, but would simply make the computation yield the wrong value.

Not the answer you're looking for? Browse other questions tagged or ask your own question.