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.