Jeff Kreeftmeijer

On the death of ifs

2013-04-29

I came across “The Death of Ifs” last week, in which @franklinwebber explains the steps he took when refactoring this piece of code to try to get rid of the if statement;

def process(input)
  if input == 'q'
    puts 'Goodbye'
  elsif input == 'tweet'
    puts 'tweeting'
  elsif input == 'dm'
    puts 'direct messaging'
  elsif input == 'help'
    puts 'helping'
  end
end

https://gist.github.com/5478520/9d509d…

The video starts out by explaining that if statements like this one might be a sign of bad design, and that this piece of code will probably grow over time and become an eyesore. While I don’t think the possibility of something growing over time and becoming an eyesore sometime in the future is a good reason to start making the code more extensible, I do believe that this code could use some refactoring.

Franklin starts out by moving some of the code to a seperate class named QuitCommand;

class QuitCommand
  def match?(input)
    input == 'q'
  end

  def exectute
    puts 'Goodbye'
  end
end

After creating a class for every one of the options in the if statement above, we have QuitCommand, TweetCommand, DirectMessageCommand and HelpCommand, and we can update the #process method we started out with to look like this;

def process(input)
  quit = QuitCommand.new
  tweet = TweetCommand.new
  dm = DirectMessageCommand.new
  help = HelpCommand.new

  if quit.match?(input) 
    quit.execute
  elsif tweet.match?(input) 
    tweet.execute
  elsif dm.match?(input)
    dm.execute
  elsif help.match?(input)
    help.execute
  end
end

https://gist.github.com/5478520/7e6ad6…

As you can see, the if statement is still there, but now the commands have all been abstracted into their own classes, which splits responsibilities and allows you to test them in isolation and extend the execution of the commands without having to touch any other parts of our application.

Franklin then gets to work on that pesky if statement;

def process(input)
  quit = QuitCommand.new
  tweet = TweetCommand.new
  dm = DirectMessageCommand.new
  help = HelpCommand.new

  commands = [quit, tweet, dm, help]

  found_command = commands.find do |command| 
    command.match?(input)
  end

  found_command.execute
end

https://gist.github.com/5478520/667704…

By putting the command instances into an array and using Enumerable#find, we can find the first instance that returns true from its #match method to get the command we need. Then we can simply call #execute on it.

But, because found_command could potentially be nil if somebody passes an unknown value, Franklin creates a new command class he names NoActionCommand that has a #match method which always returns true, and an empty #execute method. By putting it last in the commands list in #process, it will always be chosen when the other command classes don’t match. Because we can safely call #execute on an instance of NoActionCommand, the code will now work, even if input is something unexpected;

class NoActionCommand
  def match?
    true
  end

  def execute
  end
end

def process(input)
  quit = QuitCommand.new
  tweet = TweetCommand.new
  dm = DirectMessageCommand.new
  help = HelpCommand.new
  no_action = NoActionCommand.new
  
  commands = [quit, tweet, dm, help, no_action]

  found_command = commands.find do |command| 
    command.match?(input)
  end

  found_command.execute
end

https://gist.github.com/5478520/33215c…

There. No more if statements, and the code works exactly like before. Franklin splits the #process method up some more in his video, but let’s take another look to see if we can clean this up some more.

For example, doesn’t having to implement NoActionCommand show that using Enumerable#find is not the best solution to choose one of the commands? Wouldn’t it be simpler and cleaner just to make sure found_command is not nil before trying to call #execute on it? I’d probably do something like this;

def process(input)
  quit = QuitCommand.new
  tweet = TweetCommand.new
  dm = DirectMessageCommand.new
  help = HelpCommand.new

  commands = [quit, tweet, dm, help]

  found_command = command.find do |command| 
    command.match?(input)
  end

  found_command.execute if found_command
end

https://gist.github.com/5478520/fe5a04…

Not taking the if statement refactoring idea too seriously, we can get rid of our NoActionCommand class again, which removes of a lot of dummy code we don’t really need.

Going a bit further, while I agree on moving the commands to their own separate classes when they’ve grown to a size where that’s sensible, I don’t think it’s a good idea to have those classes, which are in charge of executing the commands, decide which command should be chosen for which input. Personally, I would keep the command selection in the #process method and use an a bit more functional approach;

def process(input)
  commands = {
    'q' => GoodbyeCommand.new
    'tweet' => TweetingCommand.new
    'dm' => DirectMessageCommand.new
    'help' => HelpingCommand.new
  }
  
  if command = commands[input]
    command.execute
  end
end

https://gist.github.com/5478520/3be249…

And, until there would be an actual reason to go that far, I would refactor the if statement we started out with to something that’s easier to maintain, and maybe even easier to understand. Without worrying too much about what might happen in the future, we could try to turn the code into the simplest thing that works, and that would end me up with something like this;

def process(input)
  commands = {
    'q' => 'Goodbye',
    'tweet' => 'tweeting',
    'dm' => 'direct messaging',
    'help' => 'helping'
  }

if command = commands[input] puts command end end

https://gist.github.com/5478520/eeef47…