A Real-world Example Why Magic Numbers Are Witchcraft
We all know that magic numbers are bad, but it always surprises me whenever I found out even some programmers with several years of experience in this industry still use magic numbers. This is a code snippet that I had to debug some weeks ago. This code was originally written by a programmer at my work.
program code:
if ( info.packageID == -1 || info.layerID == -1 )
{
// do something
}
Okay, we all know
-1 is a magic number here. For me, it did not take long to find out
packageID of
-1 means invalid package. Although using the constant
-1 as an invalid index is very common in programming, I personally prefer some meaningful description even for this unless it's a code for a small tool which is meant to be modified by only a handful of programmers. But I can still live with it: we programmers all tend to get lazy time to time and it's pretty straight-forward anyways.
Then what about
layerID of
-1? I just assumed it would be also the invalid index, but this case invalid layer. It is a very reasonable guess since another
-1 on the same line means invalid package, right? Wrong. It turned out to be not the case after I wasted hours of my "expensive" work hours digging other areas to fix the bug I was trying to solve. To my surprise,
layerID of
-1 means
Still Loading. What? The same number on the same line means two different things? Even in English, we get confused if one word means two different things. Even Eminem implied "what you mean is two different things" is bad in his song, Say What You Say... Okay, maybe Eminem is not so much relevant here. Anyways, you don't wanna do that in a programming language, which tends to be more confusing!
No wonder why this is a very good, or bad, example why you don't want to use magic numbers, eh?
It's All About Coding Standards, Really
In fact, It's all about coding standards, which are pretty subjective to any programmer out there: there is no one coding standard all programmers agree with. The reason why coding standards exist is to make collaboration easier: if your code is not easily understandable by your coworkers, you are being inconsiderate, and you can't be more than a dungeon programmer. In other words, you don't deserve to work at any place other than your own basement suite. Having that in mind, this is the rule I go by:
When Not to Use Magic Numbers:
- when a same constant value is showing up multiple times in code. So, if packageID of -1 is used more than once in the codebase, it's better to give it a meaningful name.
- or when the number is just too magical, thus confusing people if not explained. layerID of -1 meaning Still Loading is out of normal usage of -1 in my opinion.
- or when more than handful of programmers are editing the same codebase. If it's only a small tool which is edited only by only you. Who cares?
How Not To Be Evil
These are some of good alternatives you can use to make that code look better. All of these methods do NOT involves any run-time cost, so you should use them, if you are a programmer who cares about your coworkers.
My personal preference is the last method which uses unnamed
enum in class.
1. Use #define:
You can use good old C-style
#define to globally associate these magic numbers with meaningful names.
package.h:
#define INVALID_PACKAGE_ID -1
layer.h:
#define LAYER_STILL_LOADING -1
program code:
if ( info.packageID == INVALID_PACKAGE_ID
|| info.layerID == LAYER_STILL_LOADING )
{
// do something
}
The only downside of this method is name collision and making everything globally available does not really get along with OOP.
2. Use const int in namespace
To limit the scopes of these unmagical numbers, some programmers embed
const int into
namespace.
package.h:
namespace pacakge
{
const int INVALID_ID = -1;
...
}
layer.h:
namespace layer
{
const int STILL_LOADING = -1;
...
}
program code:
if ( info.packageID == package::INVALID_ID
|| info.layerID == layer::STILL_LOADING )
{
// do something
}
I don't particularly disagree with this method, but I have never preferred this method because it essentially stores these
const ints separate from classes.
3. Use static const int in Class
Instead of wrapping
const ints with namespace blocks, you can simple define the same
const ints as
static variable in a class.
package.h:
class pacakge
{
public:
// something else
static const int INVALID_ID;
}
package.cpp:
int package::INVALID_ID = -1;
layer.h:
class layer
{
public:
// something else
static const int STILL_LOADING;
}
layer.cpp:
int layer::STILL_LOADING = -1;
program code:
if ( info.packageID == package::INVALID_ID
|| info.layerID == layer::STILL_LOADING )
{
// do something
}
It's an okay way, but still having to define each variables in a separate
.cpp file is not so desirable.
4. Use Unnamed enum in Class
This is my preferred way, which works pretty much same as the last method, but keeps everything in one place in a clean way.
package.h:
class pacakge
{
public:
// something else
enum { INVALID_ID = -1 };
}
layer.h:
class layer
{
public:
// something else
enum { STILL_LOADING = -1 };
}
program code:
if ( info.packageID == package::INVALID_ID
|| info.layerID == layer::STILL_LOADING )
{
// do something
}
What do you think? You feel like it is much cleaner, and hopefully less evil?
