Search code examples
c++11readabilitycode-readability

Is there a better readable way to write this if statement chain?


I have the following code:

Creature::cancelWalk()
{
    Player* player = getPlayer();

    if (!player) {
        if (getMonster() && getMonster()->getMaster() && getMonster()->getMaster()->getPlayer()) {
            player = getMonster()->getMaster()->getPlayer();
        }
    }

    if (player) {
        player->sendCancelMessage(ret);
        player->sendCancelWalk();
    }
}

After a brief analysis, it's easy to understand I want to achieve something simple:

If the creature is the player itself, then sendCancelMessage and sendCancelWalk. Else, if the creature is a monster that also has a master that is a player, send the same stuff to the client.

Is there a better way to write this code without adding other methods on Monster, Creature and Player classes?

Monster and Player both are "siblings" deriving from Creature.


Solution

  • Assuming the various functions don't have different effects on multiple calls, try introducing some temporaries.

    Player* player = getPlayer();
    
    if (!player)
    {
        Monster monster = getMonster();
        if (monster)
        {
            Master *master = monster->getMaster();
            if (master) player = master->getPlayer()) {
        }
    }
    
    if (player) {
        player->sendCancelMessage(ret);
        player->sendCancelWalk();
    }
    

    Beyond that, you might want to look more carefully at your design. If you have lots of nested pointers that you need to sequentially check for NULL before dereferencing, it might be worth specifying and enforcing invariants that the pointers are not NULL (which means only construct parent objects if all the component parts can be created, and never construct objects if they can only be partially constructed).

    For example, if we assume that getMonster() returns non-NULL which guarantees that getMaster() and getPlayer() also don't return NULL ....

    Player* player = getPlayer();
    
    if (!player)
    {
        player = getMonster()->getMaster()->getPlayer());
    }
    
    if (player)
    {
        player->sendCancelMessage(ret);
        player->sendCancelWalk();
    }