Prefer Pass-by-Reference to Passing Naked Pointers as Parameters

While bug fixing, I often run across innocent looking code like:

int Warrior::getDamageAmount(const Weapon * w) const
{
    return w->getDamageAmount() + m_damageMultiplier;
}

It's functional, and a lot of game code looks similar to this snippet. A straightforward update loop might look something like:

void Player::update()
{
    Weapon * w = NULL;
    // lots and lots of code for weapon selection follows
    // ...
    // ...

    // m_playerClass may be Warrior
    int damage = m_playerClass->getDamageAmount(w);

}

Now, at some point, someone re-factors the code a little, and a seldom used path leaves w = NULL. The game crashes in code written by the author of class Warrior::getDamageAmount() when the NULL pointer is dereferenced to get the method. The Warrior class author sees the problem (NULL was passed) fixes the offending code the caller wrote and adds an assert since there's no valid thing to do when a NULL parameter is passed.

int Warrior::getDamageAmount(const Weapon * w) const
{
    assert(w != NULL);
    return w->getDamageAmount() + m_damageMultiplier;
}

While this is perfectly legal code, it could still crash in release builds. One solution (and one that safe, slower game code employs) is to check the parameters before using them and trying to do something reasonable.

int Warrior::getDamageAmount(const Weapon * w) const
{
    int result = -1;
    assert(w != NULL);
    if(w != NULL)
        result w->getDamageAmount() + m_damageMultiplier;
    return result;
}

The return value here probably isn’t reasonable. It’s a compromise: don’t crash, but live with unexpected behavior that may or may not crash the game later. Applying -1 damage effectively turns a Warrior from a damage class to a healing class! “Why is my BFG 9000 healing when it’s fired?!”

There may be a better way to communicate to the caller what’s expected when they use getDamageAmount(). If the Weapon parameter is a reference instead of a pointer, it’s implied that it can’t be NULL. If it *never* makes sense to pass NULL to a method, a parameter passed by reference is the better choice.

int Weapon::getDamageAmount(const Weapon & w) const
{
    return w.getDamageAmount() + m_damageMultiplier;
}

Now when it’s called as:
m_playerClass->getDamageAmount(*w);

and a caller attempts to dereference a NULL pointer, the crash will happen at the call site, not inside Warrior::getDamageAmount(). In other words, the programmer that made the bug immediately understands that they’ve done something inappropriate.

It’s much easier to accidentally pass a NULL pointer to a method/function that accepts a pointer. It implies that NULL is OK and may even be part of the expected behavior.

Programmers shouldn’t be expected to understand the inner workings of someone else’s code. In some cases, that implementation is opaque because the source might not be available. Most programmers *do* realize that dereferencing a NULL pointer is bad, and will do the right thing before writing incorrect code.

Using pass-by-reference provides the same performance as the first example that passes an unchecked pointer along to the method, but also provides the safety that slower assert/if/conditional checks provide in naked pointer-based code. Often, there is no right way to handle NULL and the program can't reliably recover in those cases.

If it never makes sense to have a NULL pointer as a parameter to a method, then don't use a pointer parameter, use a reference parameter instead.