Why are magic numbers bad practice?

Question

Why are magic NUMBERS considered bad practice? For example:

memcpy(ResourcesDir+GameDirLen, "/Resources", 11);

What is wrong with the 11? Why are magical numbers so bad?

Please, my question is entirely different from this one. Mine is about why they're considered bad, and the linked one is about what qualifies as one.

Answer 1

Let's start by defining a magic number.

3.14

What is that? Is it pi? If it is, it should be codified into your program as:

const double pi = 3.14;

Why?

1. The name gives it meaning beyond it being just some arbitrary number.

2. It's more readable.

   double area = pi * radius * radius;
   

3. You can change it in one place (for example, adding more digits of precision), and it will change throughout your entire program.

---

OK, since this answer seems to be getting a lot of negative attention, let's talk about enums.

Which is easier to read and easier to maintain... This?

if (trafficLightColor == 1)

or this?

if (trafficLightColor == Color.Red)

Answer 2

In short

It's a bad practice because "magic numbers" make localization and maintenance more difficult, since someone else (or a future version of you) will not know (anymore) why this magic number was as it was.

So the good practice is to avoid magic numbers. Or at least, give them a name with const or #define, and some comments to remind how they are determined. And if possible, isolate them in the code: in case of change, people will know where to look and how to change them.

What it means in practice

Your simple example requires some imagination to demonstrate the issue: Suppose the new art director of your game company decides that all the resources should from now on considered as assets. Someone might do a search/replace that will result in:

memcpy(ResourcesDir+GameDirLen, "/GameAssets", 11);    // OUCH!!!

Or maybe someone decides that resources are fun and adds some smiley (UTF8 encoded of course):

memcpy(ResourcesDir+GameDirLen, "/Ress🙂urces", 11);   // OUCH!!! x3 
                                         // (because of 4 bytes encoding instead of 1) 

Everything will compile. But in the first case you've lost your trailing '\0', which might cause buffer overflows. And in the second case, the buffer overflow is already there: your new release will ruin all the past success of your game because of security considerations and bad quality.

Your example is about a directories behind the scene. But now imagine that it's about dialogues and messages that have to be translated in several languages, each using a translation of different length...

Terminological remark

The term "magic number" has multiple meanings :

• I handled it here in the sense of "a unique unexplained constant value", and not in the other common meaning of a special integer at the beginning of a file to give a hint about the file's content.
• As there are some fierce battle about what a magic number is, it may be worth mentioning that some well known secure coding standards and organisations use the term likewise.

Answer 3

A plain number like 11 can have different meanings - it could mean “eleven things”, or it could be an error code like “thing not found”, or it could be a command code to “do this thing”, or it could be a bitmask to check several flags at once, and it can mean all of those things in the same code.

This makes maintenance a nightmare, because if you need to start counting “twelve things” but leave the other instances of 11 alone, then you can’t do a simple search and replace, you have to manually check each instance of 11.

If you ever have a situation where 11 means anything other than “eleven things”, then you are better off creating a symbolic constant for it (preprocessor macro, enumeration constant, or a const-qualified variable). The code will be easier to understand and maintain.

And to echo Andrew’s comment, make those names meaningful. If 11 is a code for a specific action, name it for that action:

#define THIS_ACTION 11
#define THAT_ACTION 12
...

If it’s an error code, name it for the error:

#define NOT_FOUND 11

etc.

Answer 4

There are two bad things with magic numbers, one of which is intrinsic to the concept and one that just is a consequence of practicalities.

The intrinsic drawback is that it's so much harder to understand. It does not convey any information except for the raw value. A named constant is much more readable.

The practical drawback is that whenever a magical number occurs in code, it's very common that it occurs at multiple places. So if you want to change it, you need to change it at several places. It's essentially code duplication. Example:

const int no_iterations = 1000;
int sum = 0;
for(int i=0; i<no_iterations; i++) {
    printf("Executing iteration %d of %d\n", i, no_iterations);
    sum += foo();
}
printf("Average value: %f\n", (double)sum/no_iterations);

Without named constants, you would have to change three occurances, which can be easy to miss. Maybe you think you could just do a search and replace for the value? Nope. Consider this code:

double sphere_volume(double r) { return r*r*r*3*pi/4; }

int main() 
{
    double spheres[4];
    // Yes, that's how to figure out the number of elemens in an array in C
    for(int i=0; i<sizeof(spheres)/sizeof(spheres[0]); i++)
        printf("Volume of sphere %f: %f\n", sphere_volume(sphere[i]);
}

Suppose you refactor the code by replacing all 4 with NO_ITER and define a #define NO_ITER 4. The code will work as before, until someone else decides that we now should change the number of iterations.

In our particular example, the code duplication is hidden. If you change the string to something else, you must ALSO remember to change the last argument. A better solution is this:

const char str[] = "/Resources";
memcpy(ResourcesDir+GameDirLen, str, sizeof(str));

Answer 5

In general, and as others have said, things used in multiple places should have a single source of truth to update when you need to change the value. The other primary reason is that a plain number without context loses semantic value. Adding a label to the number and using the label instead allows the label to provide the meaning of the number.

Some numbers have inherent semantics in many contexts. 0, 1, -1, are common examples in various contexts. Depending on context, these are fine, like i is a good generic loop counter variable name due to tradition giving it that semantic meaning.

The example you give is particularly problematic as you are essentially introducing the "cache invalidation" problem into your source code (one of two famously hard problems, the other being "naming things"; the quote is a joke, but in a "ha-ha only serious" kind of way). You have counted the number of bytes in your string in order to save a tiny amount of runtime or compile time (depending on how smart the compiler is). When (sadly, if you do this in a number of places, the answer to "if" is "yes") the string changes, it will be necessary to update the count next to it - which is surprisingly easy to overlook, especially when you've been away from that code for a while. Using the constant string as the single source of truth of its own length completely eliminates this particular source of bugs, at a very small runtime or compile time cost.

If you are working in an environment where even very small runtime costs must be minimized, the compiler is not smart enough to optimize this, you have no alternative to using a string (probably not true in such an environment), and you've tested and verified that using the constant string as a single source of truth is slowing you down... then compute the length once outside of the multiple nested loop you're in and pass it as a variable. I'm not joking. It's simply not worth the bugs it creates to hard-code string lengths.