Adding layers

²⁰/Feb 2010 By admin

Simple technique that can help in making code more future-proof and easier to modify. Applicable in most situations where two subsystems need to exchange informations. Let’s imagine the following scenario - for some reason we need to introduce ‘unlimited mana’ feature in our game. If hero is in berserker mode, his attributes increase, time slows down, he is able to cast spells even if he’s out of mana and his mana pool doesn’t deplete at all. The simplest way to implement it is to test ‘am I in berserker mode’ condition in every place when we modify mana amount. Minimal example:

if (m_hero->IsInBerserkerMode() || mana > spellCost)
{
    CastSpell();
    if (!m_hero->IsInBerserkerMode())
        mana -= spellCost;
}

It will probably work OK, however it strengthens the coupling between Spell & Hero components. Next week a designer stops by our desk and tells us about his new cool idea - mana shouldn’t be reduced if player is in berserker mode OR if he has Jewel of Mana in his inventory. We need to modify both conditions and bind both components even tighter. This will repeat itself every time we need to modify the system. How to improve it? One way is to introduce intermediate layer between both components. We can add ‘unlimited mana’ mode to spell component, which is enabled/disabled according to specified conditions (quite rarely, probably). Now, it doesn’t need to ask about it every time. Our code snippet becomes:

if (IsManaUnlimited() || mana > spellCost)
{
    CastSpell();
    if (!IsManaUnlimited())
        mana -= spellCost;
}

Now, every time we introduce a new condition we only need to modify higher level code, but the implementation responsible for casting spells stays without changes. Yes, we introduce additional variables, which is the hidden cost of this approach. It shouldn’t be choice that’s taken blindly, but in most situations shouldn’t be a problem (can be just one more flag).

Old comments

admin 2010-02-22 16:46:34

It’s not, it’s actually bound to Spell object (IsManaUnlimited() is local method). m_hero may not be the best name indeed, m_owner would be better (we have 1:1 mapping between Character & Spell).

Arowx 2010-02-22 16:00:15

The IsManaUnlimited() method is now directly tied to the m_hero object, could that be a problem if you want the same or similar behaviour for m_evilWizard?
In effect would it not be better to pass in the core object? e.g.
manaUnlimited = IsManaUnlimited(m_hero)
if (manaUnlimited || mana > spellCost)
{ …

w. 2010-02-21 02:14:07

Personally I would make:
if (CanCastSpell()) …
inline CanCastSpell() { mana>=spellCost || berserker || cheater;}
If I would see:
if (normalCodeCase || anyOtherCase) doStuff;
with anyOtherCase as just one function, i’d start wonder why (unless encapsulation) this couldn’t be in one checking function.
I prefer whole expression abstraction, but mb it’s just me.

Reg 2010-02-21 11:22:47

Good point, although such abstraction/encapsulation is a root principle in the whole programming IMHO.
If you want not to care about what the IsManaUnlimited function do, it potentially could involve some costly computations or return undeterministic (random) result, so you should call it only once and remember the result instead of calling it twice as in your code.

Garett Bass 2010-11-16 06:58:59

// Pedantic :)
bool isManaUnlimited(IsManaUnlimited()); // call once
if (isManaUnlimited || mana > spellCost)
{
CastSpell();
if (!isManaUnlimited)
mana â??= spellCost;
}

.m.m.s.

Maciej Sinilo