Optional values are a code smell

April 13, 2020

It all started with the development of a shooter game called ForKnite. I was in charge of implementing its building blocks, so I began by creating a simple Player interface:

interface Player {
  name: string;
  health: number;
}

After a while, I decided that it was time to let players carry a weapon. Given that they start unarmed, my first instinct was to do the following:

interface Player {
  name: string;
  health: number;
  weapon: Weapon | null;
}

All was bright and sunny until I had to create the actions of the game:

class GameActions {
   static void shootPlayer(from: Player, to: Player) {
        if (from.weapon === null) {
           throw Error("Weapon not found");
        }
        // Shooting logic
   }
}

I then realised that my initial implementation wasn’t scalable. I would have to constantly check if the user was carrying a weapon. Also, in this case, should the game throw an error if the user is unarmed? It didn’t seem like a valid approach to me.

That’s when I saw the real truth: Optional values are a code smell. They make you break the single responsibility principle. For example, in the shootPlayer method, there should only be logic related to shooting a player. Checking if the user carries a weapon is a different responsibility.

In fact, the more optional values, the worse it gets. Imagine that weapons could have an optional attachment:

interface Weapon {
  damage: number;
  attachment?: WeaponAttachment | null;
}

class GameActions {
   static void shootPlayer(from: Player, to: Player) {
        if (from.weapon === null) {
           throw Error("Weapon not found");
        }

        if (from.weapon.attachment) {
          // Shooting logic
        } else {
          // Shooting logic
        }
   }
}

Funnily enough, the solution to this problem is something that most developers know since the beginning of their careers: Inheritance. Instead of creating optional values, I decided to extend the different initial types:

interface Player {
  name: string;
  health: number;
}

interface ArmedPlayer extends Player {
  weapon: Weapon;
}

interface Weapon {
  damage: number;
}

interface WeaponWithExtraDamage extends Weapon {
  ...
}

class GameActions {
   static shootPlayer(from: ArmedPlayer, to: Player) {
        // Shooting logic
   }
}

That’s better, huh? With this simple action, I was able to make my code cleaner and more scalable. I was finally in peace.

PS: I’ve never written a video game. I just needed an eye-catching example.

Are optional values always a code smell?

Optional values are sometimes inevitable (see the example below), but that doesn’t mean that you shouldn’t avoid them whenever possible. You should always try to keep your code understandable and easier to work with. Be like me, be in peace :)

class PlayerRepository {
  public getPlayer(playerId: string): Player | null {
    // You could also use an "Option[Player]" monad if
    // the programming language supports it
    return Database.query(
      `SELECT * FROM Players WHERE Country='${playerId}'`
    );
  }
}