Remove else-if statement from multi-conditional logic

Question

INTRODUCTION I am studying the book Five Lines of Code (2021) by Christian Clause. The book covers several techniques regarding refactoring legacy code. I am learning much and applying several of its refactoring patterns. The book gives rules that makes a lot of sense and I have been applying them to great success.

RULE: NEVER USE IF WITH ELSE

is one that I agree with, and I am trying to use apply it in my code. However, there is a situation that I have found where I cannot refactor - multi-conditioned logic. Now, I am a hobbyist programmer and I have been studying refactoring and design patterns earnestly for the last six months and definitely am not an expert/professional.

PROBLEM In my Dart code, which I have reproduced using a simplified example in DartPad, I have commented where I am having the difficulty in refactoring.

QUESTION I am wondering what is the best way to refactor the following code to eliminate the else-if statement without using a switch statement? Ideally, I would like to use a design pattern. An example solution would be most appreciated.

void main() {
  List<BoardPeice> tiles = [];
  var pacman = Player(name: 'PacMan');
  var ghost = Ghost(name: 'Red ghost');
  var lvl1 = Game()..isStarted = true;

  for (int i = 0; i < 20; i++) {
    tiles.add(drawMap(pacman, ghost, lvl1, i));
  }

  for (int i = 0; i < tiles.length; i++) {
    print(tiles[i]);
  }
}

BoardPeice drawMap(Character player, Character ghost, Game game, int index) {
  // TODO: else-if statement that I would like to refactor.
  if (player.location == index) {
    return PlayerTile(hero: player);
  } else if (ghost.location == index) {
    return GhostTile(minion: ghost);
  } else if (game.barriers.contains(index)) {
    return BlueBarrierTile(index: index);
  } else if (game.pellets.contains(index) || !game.isStarted) {
    return FoodTokenTile(index: index);
  } else {
    return EmptyTile(index: index);
  }
}

abstract class Character {
  Character({required this.name});
  String name;
  late int location;
  late String color;
}

class Player implements Character {
  Player({required this.name});
  @override
  String name;
  @override
  int location = 11;
  @override
  String color = 'Gold';
}

class Ghost implements Character {
  Ghost({required this.name});
  @override
  String name;
  @override
  int location = 12;
  @override
  String color = 'Black';
}

abstract class BoardPeice {
  String? name;

  @override
  String toString() => name!;
}

class PlayerTile extends BoardPeice {
  PlayerTile({required this.hero}) {
    name = hero.name;
  }

  final Character hero;
}

class GhostTile extends BoardPeice {
  GhostTile({required this.minion}) {
    name = minion.name;
  }

  final Character minion;
}

class BlueBarrierTile extends BoardPeice {
  BlueBarrierTile({required this.index}) {
    name = 'barrier: ${index.toString()}';
  }

  final int index;
}

class FoodTokenTile extends BoardPeice {
  FoodTokenTile({required this.index}) {
    name = 'pellet: ${index.toString()}';
  }

  final int index;
}

class EmptyTile extends BoardPeice {
  EmptyTile({required this.index}) {
    name = 'empty: ${index.toString()}';
  }

  final int index;
}

class Game {
  var barriers = [0, 1, 2, 3, 4, 5];
  var pellets = [6, 7, 8, 9, 10];
  var isStarted = false;
}

REFACTORED CODE For completeness, I include the refactored code according to the comments.

void main() {
  List<BoardPiece> tiles = [];
  var pacman = Player(name: 'PacMan');
  var ghost = Ghost(name: 'Red ghost');
  var lvl1 = Game()..isStarted = true;

  for (int i = 0; i < 20; i++) {
    tiles.add(drawMap(pacman, ghost, lvl1, i));
  }

  for (int i = 0; i < tiles.length; i++) {
    print(tiles[i]);
  }
}

List<BoardPieceStrategy> boardPieceStrategies = [
  PlayerTileCheck(),
  GhostTileCheck(),
  BlueBarrierTileCheck(),
  FoodTileCheck(),
  DefaultTileCheck(),
];

class Context {
  Character player;
  Character ghost;
  Game game;
  int index;
  Context({
    required this.player,
    required this.ghost,
    required this.game,
    required this.index,
  });
}

abstract class BoardPieceStrategy {
  bool isRelevant(Context context);
  BoardPiece getTile(Context context);
}

//strategy objects
class GhostTileCheck implements BoardPieceStrategy {
  @override
  bool isRelevant(Context context) {
    return context.ghost.location == context.index;
  }

  @override
  BoardPiece getTile(Context context) {
    return GhostTile(minion: context.ghost);
  }
}

class DefaultTileCheck implements BoardPieceStrategy {
  @override
  bool isRelevant(Context context) {
    return true;
  }

  @override
  BoardPiece getTile(Context context) {
    return EmptyTile(index: context.index);
  }
}

class PlayerTileCheck implements BoardPieceStrategy {
  @override
  bool isRelevant(Context context) {
    return context.player.location == context.index;
  }

  @override
  BoardPiece getTile(Context context) {
    return PlayerTile(hero: context.player);
  }
}

class BlueBarrierTileCheck implements BoardPieceStrategy {
  @override
  bool isRelevant(Context context) {
    return context.game.barriers.contains(context.index);
  }

  @override
  BoardPiece getTile(Context context) {
    return BlueBarrierTile(index: context.index);
  }
}

class FoodTileCheck implements BoardPieceStrategy {
  @override
  bool isRelevant(Context context) {
    return context.game.pellets.contains(context.index);
  }

  @override
  BoardPiece getTile(Context context) {
    return FoodTile(index: context.index);
  }
}

BoardPiece drawMap(Character player, Character ghost, Game game, int index) {
  var context = Context(player: player, ghost: ghost, game: game, index: index);
  var boardPieceStrategy = boardPieceStrategies
      .firstWhere((strategy) => strategy.isRelevant(context));
  return boardPieceStrategy.getTile(context);
}

abstract class Character {
  Character({required this.name});
  String name;
  late int location;
  late String color;
}

class Player implements Character {
  Player({required this.name});
  @override
  String name;
  @override
  int location = 11;
  @override
  String color = 'Gold';
}

class Ghost implements Character {
  Ghost({required this.name});
  @override
  String name;
  @override
  int location = 12;
  @override
  String color = 'Black';
}

abstract class BoardPiece {
  String? name;

  @override
  String toString() => name!;
}

class PlayerTile extends BoardPiece {
  PlayerTile({required this.hero}) {
    name = hero.name;
  }

  final Character hero;
}

class GhostTile extends BoardPiece {
  GhostTile({required this.minion}) {
    name = minion.name;
  }

  final Character minion;
}

class BlueBarrierTile extends BoardPiece {
  BlueBarrierTile({required this.index}) {
    name = 'barrier: ${index.toString()}';
  }

  final int index;
}

class FoodTile extends BoardPiece {
  FoodTile({required this.index}) {
    name = 'pellet: ${index.toString()}';
  }

  final int index;
}

class EmptyTile extends BoardPiece {
  EmptyTile({required this.index}) {
    name = 'empty: ${index.toString()}';
  }

  final int index;
}

class Game {
  var barriers = [0, 1, 2, 3, 4, 5];
  var pellets = [6, 7, 8, 9, 10];
  var isStarted = false;
}

Answer 1

In these situations, I like to use a mix of the strategy pattern and the chain of command pattern.

I define an interface with two methods: a predicate whose boolean result tells me if the strategy is applicable to the parameter (this replaces the if condition), and the main method that returns the result.

(I don't know Dart, so please excuse syntax errors or non-idiomatic code -- I trust you can translate the concepts into something more suitable.)

class Context {
  Character player;
  Character ghost;
  Game game;
  int index;
  Context(this.player, this.ghost, this.game, this.index) {
  }
}

abstract class BoardPieceStrategy {
  bool isRelevant(Context context);
  BoardPiece getTile(Context context);
}

Each if block gets its own strategy object. For example:

class GhostTileCheck implements BoardPieceStrategy {
  bool isRelevant(Context context) {
    return context.ghost.location == context.index;
  }

  BoardPiece getTile(Context context) {
    return GhostTile(minion: context.ghost);
  }
}

class DefaultTileCheck implements BoardPieceStrategy {
  bool isRelevant(Context context) { return true; }
  BoardPiece getTile(Context context) {
    return EmptyTile(index: context.index);
  }
}

Now, instead of your if/else, you make an array of the strategies and iterate over them to select the first applicable one.

var boardPieceStrategies = <BoardPieceStrategy>[
  // one of these for each if block you had, and put them in order
  new GhostTileCheck(),
  // ...
  new DefaultTileCheck()
];

BoardPiece drawMap(Character player, Character ghost, Game game, int index) {
  var context = Context(player, ghost, game, index);
  var boardPieceStrategy = boardPieceStrategies.firstWhere((strategy) => strategy.isRelevant(context));
  return boardPieceStrategy.getTile(context);
}

Answer 2

I don't think that rule should be followed that strictly. Usually it's just meant as a hint to use the Strategy pattern, instead of putting lots of different logic in one if structure.

But here you simply want to map the content of the board to a graphical element. An if-else or a switch case is just the core logic of that simple task. There is no sense in trying to hide that, it would just make the code harder to read without any benefit.

To make your code a bit nicer I would create a function to get the tile type at an index position of your board.

Once you have that the code to pick the corresponding graphical tile could be made more readable by using a (Hash)Map.

Otherwise you would have to change the way how the positions of your board tiles are stored. I.e. By letting the board store tiles that know how to render themselves.

Answer 3

RULE: NEVER USE IF WITH ELSE

refactor the following code to eliminate the else-if statement without using a switch statement?

BEFORE

BoardPeice drawMap(Character player, Character ghost, Game game, int index) {
     // TODO: else-if statement that I would like to refactor.
  if (player.location == index) {
    return PlayerTile(hero: player);
  } else if (ghost.location == index) {
    return GhostTile(minion: ghost);
  } else if (game.barriers.contains(index)) {
    return BlueBarrierTile(index: index);
  } else if (game.pellets.contains(index) || !game.isStarted) {
    return FoodTokenTile(index: index);
  } else {
    return EmptyTile(index: index);
}

AFTER

BoardPeice drawMap(Character player, Character ghost, Game game, int index) {
  if (player.location == index) { return PlayerTile(hero: player); }
  if (ghost.location == index)  { return GhostTile(minion: ghost); }
  if (game.barriers.contains(index)) { return BlueBarrierTile(index: index); }
  if (game.pellets.contains(index) || !game.isStarted) { return FoodTokenTile(index: index); }

  return EmptyTile(index: index);
}

Why over-engineer something so simple?

However, if your instinct was to get rid of the clumped ifs altogether, that's good because this if clog is a symptom of poor class cohesion. The solution is simple - A Character.Draw override method. Now a Player, Ghost, etc. can draw itself and is called without asking IF first.

class Player implements Character {
  @override
  Tile Draw { return PlayerTile(hero: this); }
}

abstract class BoardPiece {
   Character OccupiedBy = null;

   // no override needed!
   Tile Draw {
     return OccupiedBy == null ? new Tile() : OccupiedBy.Draw() ;
   }
}