Dec 26, 2008

No Magic Numbers Please

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?

1 comment:

  1. Hi, Pope
    I heard about you from PGPStudy in Korea. I'm planning to land to Vancouver and to study Visual & Game Programming in AI Vancouver. I read your profile and I am surprised you are teaching in AI Vancouver. So can I get some information and advice about AI Vancouver, etc...? Would you let me know your email?
    My email is yeebw@hotmail.com

    I'm sorry about this strange comment. but it's only way for me to contact to you now. It's OK to delete this comment if you want.

    ReplyDelete