37

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?

oosterwal
  • 1,713
  • 14
  • 16
  • 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 09 '11 at 16:31
  • 17
    `TimeSpan.FromDays(numDays).Seconds;` – Nobody Mar 09 '11 at 16:34
  • @rmx: That's even better (when such a function is available). – FrustratedWithFormsDesigner Mar 09 '11 at 16:35
  • Sorry, should have mentioned - C# – Nobody Mar 09 '11 at 16:40
  • @FrustratedWithFormsDesigner: Your point is absolutely valid. I was using this example as a close-to-the-edge scenario where it could, arguably, fall on either side of the argument. HOURS_PER_DAY will never need to be altered, so that minimizes the need to create a macro or const, as compared to a value that probably won' – oosterwal Mar 09 '11 at 18:28
  • 21
    @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 09 '11 at 18:29
  • @FrustratedWithFormsDesigner: [sorry about the interruption] ...probably won't ever change, but could, such as the elevation of a given city. My rebuttal question is: What does the body of the CALC_SECONDS() function or macro look like? Do you hard-code those values or use macros (assume a less trivial case than the one given)? --your observation about my code in the defunct Mars Lander is duly noted ;-) – oosterwal Mar 09 '11 at 18:40
  • 26
    I would have reduced the number of constants to just SECONDS_PER_DAY=86400. Why calculate something that won't change? – JohnFx Mar 09 '11 at 18:41
  • 1
    @oosterwal: very good point. The value has to be somewhere. If it were going to be configurable, a .properties file might do. Loaded once at application startup. – FrustratedWithFormsDesigner Mar 09 '11 at 18:43
  • @JohnFx: I agree completely with your your observation. As I mentioned in another comment, the example in the original question was meant as starting point for discussion. – oosterwal Mar 09 '11 at 18:56
  • 19
    What about leap seconds? – John Mar 09 '11 at 23:41
  • 2
    @rmx, that will give 0. You need to use `TotalSeconds`. – Peter Taylor Mar 10 '11 at 07:22

10 Answers10

47

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;
kevin cline
  • 33,608
  • 3
  • 71
  • 142
  • 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 09 '11 at 19:27
  • 46
    Or even worse: `publid final int FOUR = 3;` – gablin Mar 09 '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 08:09
  • 2
    @gablin: To be fair, it is quite useful for pubs to have lids. – Alan Pearce Mar 10 '11 at 09:24
  • @kevin "THREE = 3" Yikes. I old nightmares. I had the unfortunate pleasure of calling contractors out on nearly exlusively using magic number literals scattered throughout during a code review. Their set of corrections was exactly as your "don't do." Every literal value was spelled out. Needless to say they were fired, but only after more shenanigans. – JustinC Mar 11 '11 at 06:20
  • 11
    I've seen this: `public static int THREE = 3;` ... note - no `final`! – Stephen C May 09 '11 at 04:39
  • 1
    Well, arguably, the number of seconds in a day changes all the time. Just not very much. – DeadMG Jul 21 '14 at 16:47
  • @oosterwal my last professional embedded software job had pages of `.h` files filled with CRAZY_CONSTANTS_AT_LEAST_THIS_LONG. :/ But I fixed their Y2K bug by altering the PowerPC machine code buried in the VXworks package. – Krista K Dec 18 '14 at 12:14
  • Linked from a recently asked question. +1 just for the final "Do not do this". – David Hammen Apr 10 '17 at 05:06
32

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.

700 Software
  • 263
  • 1
  • 9
Vitor Py
  • 4,838
  • 1
  • 27
  • 33
  • 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 09 '11 at 16:45
  • 4
    @btilly I wholeheartedly agree with you. But it happens, often due to external factors. – Vitor Py Mar 09 '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 09 '11 at 17:09
  • @vitor-braga - Usually it happens due to internal factors, and blame is placed on external factors. The biggest internal factor is frequently management that only understands how to motivate by providing schedule pressure. I believe that http://www.amazon.com/Rapid-Development-Taming-Software-Schedules/dp/1556159005 has some very useful things to say on this. – btilly Mar 09 '11 at 18:20
  • 9
    @btilly: Peak is 35-40 hours or a blood alchohol level between 0.129% and 0.138%. I read it on [XKCD](http://imgs.xkcd.com/comics/ballmer_peak.png), so it's got to be true! – oosterwal Mar 09 '11 at 18:46
  • Do you have to be at peak to create a bunch of constants? – JeffO Mar 10 '11 at 03:03
  • 1
    @Vitor - Consider `seconds_from_days()` rather than `CONVERT_DAYS_TO_SECONDS` as `seconds = seconds_from_days(num_days)` gives better in-line readability where similar units are kept together. – Mark Booth Apr 16 '11 at 23:11
  • @Mark Booth I agree. – Vitor Py Apr 16 '11 at 23:58
  • Better still is "Duration d = Duration.fromDays(n); ... d.getSeconds(); – kevin cline Sep 18 '17 at 11:39
9

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

bkulyk
  • 191
  • 2
  • 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
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.

FrustratedWithFormsDesigner
  • 46,105
  • 7
  • 126
  • 176
  • 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 09 '11 at 16:36
  • I strongly disagree. If your controls need to be aligned in a certain way, then you definitely should be using named constants. If, for instance, two controls are aligned horizontally, then their y-coordinate should be the same. So it should be a constant. – Dima Mar 09 '11 at 16:37
  • 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 09 '11 at 16:39
  • @rmx: I agree with this too. Using 0 or 1 to check resultset/list sizes, or as bounds on a loop is usually OK. – FrustratedWithFormsDesigner Mar 09 '11 at 16:40
  • 5
    Let's not confuse code meant to be generated and handled by a tool with code written for human consumption though. – biziclop Mar 09 '11 at 18:45
  • 2
    @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 09 '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 09 '11 at 19:11
  • @oosterwal: That would probably be stored in a .properties or .config file, loaded by the application at startup (or something like that). Your example would be a magic number => not good. ;) – FrustratedWithFormsDesigner Mar 09 '11 at 19:16
  • 1
    @Dima: I agree with you. I should have made it more clear that if you're hand-crafting UI layout code, it's probably best to avoid constants (and even if you have to introduce a "fudge-factor" that should be defined as a constant too somewhere). – FrustratedWithFormsDesigner Mar 09 '11 at 19:23
  • 1
    Why write the awkward phrase "DAYS_TO_SECONDS_FACTOR" when standard English already have a perfectly good, unambiguous name for that number: "SECONDS_PER_DAY". – kevin cline May 23 '14 at 20:52
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.

Elias Zamaria
  • 154
  • 1
  • 10
biziclop
  • 3,351
  • 21
  • 22
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
dan04
  • 3,748
  • 1
  • 24
  • 26
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.

Neil
  • 22,670
  • 45
  • 76
  • 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 09 '11 at 19:18
  • Then why not: seconds = num_days * 86400 ? That won't change either. – JeffO Mar 10 '11 at 02: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 08: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

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.

Martijn Pieters
  • 14,499
  • 10
  • 57
  • 58
Kux
  • 131
  • 3
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.

Carra
  • 4,261
  • 24
  • 28
  • 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 09 '11 at 19:22
  • 1
    Writing 86400 is very different from writing 24 * 60 * 60. – Carra Mar 09 '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 05:56
  • 1
    @Dave, excellent point. Leap seconds exist - http://en.wikipedia.org/wiki/Leap_second –  Mar 10 '11 at 06:18
  • Fair point. Adding a function would be a safeguard if you ever need to catch those exceptions. – Carra Mar 10 '11 at 09:42
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.

Community
  • 1
guiman
  • 2,088
  • 13
  • 17