36

We're all aware that magic numbers (hard-coded values) can wreak havoc in your program, especially when it's time to modify a section of code that has no comments, but where do you draw the line?

For instance, if you have a function that calculates the number of seconds between two days, do you replace

seconds = num_days * 24 * 60 * 60

with

seconds = num_days * HOURS_PER_DAY * MINUTES_PER_HOUR * SECONDS_PER_MINUTE

At what point do you decide that it is completely obvious what the hard-coded value means and leave it alone?

|improve this question
  • 2
    Why not replace that calculation with a function or macro so your code ends up looking like seconds = CALC_SECONDS(num_days); – FrustratedWithFormsDesigner Mar 9 '11 at 16:31
  • 15
    TimeSpan.FromDays(numDays).Seconds; – Nobody Mar 9 '11 at 16:34
  • 18
    @oosterwal: With that attitude (HOURS_PER_DAY will never need to be altered), you'll never be coding for software deployed on Mars. :P – FrustratedWithFormsDesigner Mar 9 '11 at 18:29
  • 23
    I would have reduced the number of constants to just SECONDS_PER_DAY=86400. Why calculate something that won't change? – JohnFx Mar 9 '11 at 18:41
  • 17
    What about leap seconds? – John Mar 9 '11 at 23:41

10 Answers 10

40

There are two reasons to use symbolic constants instead of numeric literals:

  1. To simplify maintenance if the magic numbers change. This does not apply to your example. It is extremely unlikely that the number of seconds in an hour, or the number of hours in a day will change.

  2. To improve readibility. The expression "24*60*60" is pretty obvious to almost everyone. "SECONDS_PER_DAY" is too, but if you are hunting a bug, you may have to go check that SECONDS_PER_DAY was defined correctly. There is value in brevity.

For magic numbers that appear exactly once, and are independent of the rest of the program, deciding whether to create a symbol for that number is a matter of taste. If there is any doubt, go ahead and create a symbol.

Do not do this:

public static final int THREE = 3;
|improve this answer
  • 3
    +1 @kevin cline: I agree with your point about brevity a bug-hunting. The added benefit I see to using a named constant, especially when debugging, is that if it is discovered that a constant was defined incorrectly you only need to change one piece of code rather than searching through a whole project for all occurrences of the incorrectly implemented value. – oosterwal Mar 9 '11 at 19:27
  • 40
    Or even worse: publid final int FOUR = 3; – gablin Mar 9 '11 at 20:44
  • 3
    Oh dear, you must have worked with the same guy I once worked with. – quickly_now Mar 10 '11 at 8:09
  • 2
    @gablin: To be fair, it is quite useful for pubs to have lids. – Alan Pearce Mar 10 '11 at 9:24
  • 10
    I've seen this: public static int THREE = 3; ... note - no final! – Stephen C May 9 '11 at 4:39
29

I'd keep the rule of never having magic numbers.

While

seconds = num_days * 24 * 60 * 60

Is perfectly readable most of the time, after having coded for 10 hours a day for three or four weeks in crunch mode

seconds = num_days * HOURS_PER_DAY * MINUTES_PER_HOUR * SECONDS_PER_MINUTE

is much easier to read.

FrustratedWithFormsDesigner's suggestion is better:

seconds = num_days * DAYS_TO_SECOND_FACTOR

or even better

seconds = CONVERT_DAYS_TO_SECONDS(num_days)

Things stop being obvious when you're very tired. Code defensively.

|improve this answer
  • 13
    Getting into a crunch mode like you describe is a counterproductive antipattern that should be avoided. Programmers reach peak sustained productivity at about 35-40 hours/week. – btilly Mar 9 '11 at 16:45
  • 4
    @btilly I wholeheartedly agree with you. But it happens, often due to external factors. – Vitor Py Mar 9 '11 at 16:49
  • 3
    I general define constants for second, minute, day and hour. If nothing else '30 * MINUTE' is really easy to read and I know its a time without having to think about it. – Zachary K Mar 9 '11 at 17:09
  • 9
    @btilly: Peak is 35-40 hours or a blood alchohol level between 0.129% and 0.138%. I read it on XKCD, so it's got to be true! – oosterwal Mar 9 '11 at 18:46
  • 1
    If I saw a constant like HOURS_PER_DAY I would delete it and then publicly humiliate you in front of your peers. Ok, maybe I would forgo the public humiliation, but I would probably delete it. – Ed S. Mar 10 '11 at 3:30
8

The time to say no is almost always. Times where I find it's easier to just use hard-coded numbers is in places such as UI layout - creating a constant for the positioning of every control on the form gets very cubmersone and tiring and if that code is usually handled by a UI designer it doesn't much matter. ...unless the UI is laid out dynamically, or uses relative positions to some anchor or is written by hand. In that case, I'd say it's better to define some meaningful constants for layout. And if you need a fudge factor here or there to align/position something "just right", that should also be defined.

But in your example, I do think that replacing 24 * 60 * 60 by DAYS_TO_SECONDS_FACTOR is better.

I concede that hard-coded values are also OK when the context and usage is completely clear. This, however, is a judgement call...

Example:

As @rmx pointed out, using 0 or 1 to check if a list is empty, or maybe in a loop's bounds is an example of a case where the purpose of the constant is very clear.

|improve this answer
  • 2
    It's usually OK to use 0 or 1 I reckon. if(someList.Count != 0) ... is better than if(someList.Count != MinListCount) .... Not always, but generally. – Nobody Mar 9 '11 at 16:36
  • 2
    @Dima: VS forms designer handles all that. If it wants to create constants, that's fine with me. But I'm not going into the generated code and replacing all hard-coded values with constants. – FrustratedWithFormsDesigner Mar 9 '11 at 16:39
  • 4
    Let's not confuse code meant to be generated and handled by a tool with code written for human consumption though. – biziclop Mar 9 '11 at 18:45
  • 1
    @FrustratedWithFormsDesigner as @biziclop pointed out, generated code is an entirely different animal. Named constants absolutely must be used in code that is read and modified by people. Generated code, at least in the ideal case, should not be modified at all. – Dima Mar 9 '11 at 19:09
  • 2
    @FrustratedWithFormsDesigner: What happens when you have a well-known value hard-coded in dozens of files in your program that suddenly needs to be changed? For instance, you hard-code the value representing the number of clock-ticks per microsecond for an embedded processor then are told to port your software to a design where there are a different number of clock-ticks per microsecond. If your value had been something common, like 8, performing a find/replace on dozens of files could end up causing more problems. – oosterwal Mar 9 '11 at 19:11
8

Stop when you can't pin down a meaning or purpose to the number.

seconds = num_days * HOURS_PER_DAY * MINUTES_PER_HOUR * SECONDS_PER_MINUTE

is much easier to read than just using the numbers. (Although it could be made more readable by having a single SECONDS_PER_DAY constant, but it's a completely separate issue.)

Assume that a developer looking at the code can see what it does. But do not assume that they also know why. If your constant helps understanding the why, go for it. If not, don't.

If you'd end up with too many constants, as was suggested by one answer, consider using an external configuration file instead, as having dozens of constants in a file doesn't exactly improve readability.

|improve this answer
7

I'd probably say "no" to things like:

#define HTML_END_TAG "</html>"

And definitely would say "no" to:

#define QUADRATIC_DISCRIMINANT_COEF 4
#define QUADRATIC_DENOMINATOR_COEF  2
|improve this answer
7

One of the best examples I have found for promoting use of constants for obvious thing like HOURS_PER_DAY is:

We were calculating how long things were sitting in a person's job queue. The requirements were loosely defined and the programmer hard coded 24 in a number of places. Eventually we realized that it wasn't fair to punish the users for sitting on a problem for 24 hours when the really only work for 8 hours a day. When the task came to fix this AND see what other reports might have the same problem it was pretty difficult to grep/search through the code for 24 would have been much easier to grep/search for HOURS_PER_DAY

|improve this answer
  • oh noes, so hours per day varies according to whether you refer to working hours per day (I work 7.5 BTW) or hours in a day. To change the meaning of the constant like that, you'd want to replace the name of it to something else. Your point about searching made easy is valid though. – gbjbaanb Feb 28 '12 at 21:05
  • 2
    It appears that in this case HOURS_PER_DAY was not really the desired constant either. But being able to search for it by name is a huge benefit, even if (or especially if) you need to change it to something else in many places. – David K Jul 21 '14 at 17:41
4

I think that so long as the number is completely constant and has no possibility of changing, it's perfectly acceptable. So in your case, seconds = num_days * 24 * 60 * 60 is fine (assuming of course that you don't do something silly like do this sort of calculation inside a loop) and arguably better for readability than seconds = num_days * HOURS_PER_DAY * MINUTES_PER_HOUR * SECONDS_PER_MINUTE.

It's when you do things like this that's bad:

lineOffset += 24; // 24 lines to a page

Even if you couldn't fit anymore lines on the page or even if you have no intentions of changing it, use a constant variable instead, because one day it's going to come back to haunt you. Ultimately, the point is readability, not saving 2 cycles of calculation on the CPU. This is no longer 1978 when precious bytes were squeezed for all their worth.

|improve this answer
  • 2
    Would you find it acceptable to hard-code the unchanging value 86400 instead of using the named constant SECONDS_PER_DAY? How would you verify that all occurrences of the value were correct and not missing a 0 or swapping the 6 ad 4, for instance? – oosterwal Mar 9 '11 at 19:18
  • Then why not: seconds = num_days * 86400 ? That won't change either. – JeffO Mar 10 '11 at 2:56
  • 2
    I've done the SECONDS_PER_DAY thing both ways - using the name and using the number. When you come back to the code 2 years later, the named number ALWAYS makes more sense. – quickly_now Mar 10 '11 at 8:12
  • 2
    seconds = num_days * 86400 isn't clear to me. That's what ultimately counts. If I see "seconds = num_days * 24 * 60 * 60", aside from the fact that the variable names lend the meaning fairly well in this instance, I'd immediately ask myself why I separated them and the meaning becomes obvious because I left them as numbers (hence they're constant), not variables to which further investigation would require me to understand their values and if they are constant. – Neil Mar 10 '11 at 16:55
  • 1
    What people often don't realise: If you change that lineOffset value from 24 to 25, you'll have to go through all of your code to see where 24 was used and if it needs changing, and then all the days to hours calculations multiplying by 24 really get into your way. – gnasher729 Jan 24 '17 at 18:23
3
seconds = num_days * 24 * 60 * 60

Is perfectly fine. These aren't really magic numbers as they will never change.

Any numbers that can reasonably change or have no obvious meaning should be put into variables. Which means pretty much all of them.

|improve this answer
  • 2
    Would seconds = num_days * 86400 still be acceptable? If a value like that were used multiple times in many different files, how would you verify that someone had not accidentally typed seconds = num_days * 84600 in one or two places? – oosterwal Mar 9 '11 at 19:22
  • 1
    Writing 86400 is very different from writing 24 * 60 * 60. – Carra Mar 9 '11 at 21:59
  • 4
    Of course it will change. Not every day has 86,400 seconds in it. Consider, for example, daylight savings time. Once a year, some places only have 23 hours in a day, and another day they'll have 25. poof your numbers are broken. – Dave DeLong Mar 10 '11 at 5:56
  • 1
    @Dave, excellent point. Leap seconds exist - en.wikipedia.org/wiki/Leap_second – user1249 Mar 10 '11 at 6:18
  • Fair point. Adding a function would be a safeguard if you ever need to catch those exceptions. – Carra Mar 10 '11 at 9:42
3

I would avoid creating constants (magic values) to convert a value from one unit to other. In case of converting I prefer a speaking method name. In this example this would be e.g. DayToSeconds(num_days) internal the method don't need magic values because, the meaning of "24" and "60" is clear.

In this case I'd never use seconds / minutes / hours. I would only use TimeSpan/DateTime.

|improve this answer
1

Use context as a parameter to decide

For example, you have a function called "calculateSecondsBetween: aDay and: anotherDay", you won't need to do much exaplanation about what those numbers do, because the function name is quite representative.

And another question is, which are the possibilities to calculate it in a different way? Sometimes there are lots of ways to do the same thing, so to guide future programmers and show them what method you used, defining constants could help figure it out.

|improve this answer

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