How best to refactor action with too many conditionals?

Question

A colleague and I are working on a RoR project which receives text messages from the Twilio messaging service and then parses them to perform commands. To interface with Twilio, we have a controller action that Twilio POSTs to with a few parameters, like to, from, and message body.

The problem is that we have a lot of possible commands that the user can perform by texting in. Eventually, our code was starting to look like this:

# message_controller.rb

def process_message
  words = params[:Body].upcase.split

  if words[0] == 'FOO'
    do_something()
    render 'foo'
  elsif words[0] == 'BAR' && params[:From] == 'some number'
    .
    . # 10 to 15 lines
    .
  else
    render 'command_not_found'
  end
end

We decided to refactor using the Interpreter pattern and parse the message body sort of like a language. We ended up with something like this in the controller:

# message_controller.rb

def process_message
  interpreter = InitialInterpreter.new(message_params)

  while !interpreter.is_terminal? do
    interpreter = interpreter.next_interpreter()
  end

  render interpreter.template
end

def message_params
  params.permit(:To, :From, :Body)
end

And we created models like this:

# initial_interpreter.rb, a non-terminal interpreter

attr_accessible :To, :From, :Body

def next_interpreter
  if words[0] == 'FOO'
    FooInterpreter.new
  elsif
    .
    . # a couple of non-terminal interpreters
    .
  else
    CommandNotFoundInterpreter.new
  end
end

# command_not_found_interpreter.rb, a terminal interpreter

def is_terminal?
  true
end

def template
  'command_not_found'
end

But then we started thinking about it and realized that almost all our commands have no more than two expressions, so we didn't really need the while loop and refactored like this:

# message_controller.rb

def process_message
  interpreter = InitialInterpreter.new(message_params)

  render interpreter.template
end

# initial_interpreter.rb

def template
  if words[0] == 'FOO'
    interpreter = FooInterpreter.new
  elsif
    .
    . # a couple of non-terminal interpreters
    .
  else
    interpreter = CommandNotFoundInterpreter.new
  end

  interpreter.template
end

# command_not_found_interpreter.rb

def template
  'command_not_found'
end

But this seems a bit too simple. Now, it's just classes wrapping method. Are we overthinking this? Does this pattern seem good or bad? I feel like we're doing something wrong, but I can't put my finger on it. What would be best way of dealing with this sort of case?

EDIT

I also should have mention that I'm wondering about efficiency too. This solution we're using doesn't seem very efficient, and this action may be receiving a lot of traffic when we go live. I wonder if there's anything rails or ruby specific that might provide a good solution.

Answer 1

In my opinion, your pattern seems fine. I've done something similar in the past and haven't had any bad experiences with it. If you really have a ton of cases, maybe the observer pattern would be okay here. Your InitialInterpreter can accept the message and dispatch it to all of it's observers (which would be your other interpreters). Each interpreter can choose to do something with the message or not based on regular expressions or string matching or whatever you choose.

In this pattern, interpreters contain the logic of when to interpret inside of themselves. The InitialInterpreter and your controller won't care what interpreter picks up the message as long as it's subscribed to the InitialInterpreter's events.

Another benefit to the observer pattern is that it allows you to easily add multiple interpreters for a single command if you need to do so in the future. Ruby has built in support for observers via the Observable module. I believe Rails has something for it as well but I've never personally used it.