Controller method to manage SMS subscriptions

Question

I have a huge method in my controller that is not DRY and just could use some general TLC. I'm new to ruby and I have not refactored my own code before, only contrived code you find on tutorials. Below I will show my controller, just to be clear I'm not looking for a full answer just maybe where I should start and some helpful tips.

def process_message(message, subscriber)
 if message == "tulip" || message == "stem"
 # If the user has subscribed flip the bit
 # and let them know
 subscribed = message == "tulip"
 subscriber.update(subscribed: subscribed)
 # Respond appropriately
 output = "You are now subscribed for updates."
 unless subscriber.subscribed
 output = "You have unsubscribed from notifications. Test 'TULIP' to start receieving updates again"
 end
 subscriber.add_to_group("visitor")
 elsif message == "dancers"
 subscribed = message == "dancers"
 subscriber.update(subscribed: subscribed)
 subscriber.add_to_group("dancers")
 output = "You have been subscribed to the Dancers list"
 elsif message == "staff"
 subscribed = message == "staff"
 subscriber.update(subscribed: subscribed)
 subscriber.add_to_group("staff")
 output = "You have been subscribed to the Staff list"
 elsif message == "volunteer"
 subscribed = message == "volunteer"
 subscriber.update(subscribed: subscribed)
 subscriber.add_to_group("volunteer")
 output = "You have been subscribed to the Volunteer list"
 else
 # If we don't recognize the command, text back with the list of
 # available commands
 output = "Sorry, we don't recognize that command. Available commands are: 'TULIP' or 'STEM'."
 end
 output

• Sergio Tulentsev: What about message "stem"? Is such user not subscribed?
• Cameron Bass: Yes. That will change but not of major importance for the moment.
• toddmetheny: Couple things: I would refactor into a method in your model and call it here and then return the output. I would also alter the conditional to a case/when statement so you don't have to keep repeating message.
• toddmetheny: what's the associated model name for this controller? will post some further notes/examples as an answer.
• Cameron Bass: person.rb is what the create method uses.

Answer 1

What I noticed is that there are very few differences between what's happening within the sections that the 'dancers', 'staff', and 'volunteer' messages take you through. Because the only thing that really changed between the three was the word within the message itself, I decided to combine them and use the message variable.

def process_message(message, subscriber)
 if standard_groups.include?(message)
 subscriber.update(subscribed: true)
 subscriber.add_to_group(message)
 "You have been subscribed to the #{message.capitalize} list"
 elsif message == 'tulip' || message == 'stem'
 subscriber.update(subscribed: message == 'tulip')
 subscriber.add_to_group('visitor')
 return 'You are now subscribed for updates.' if subscriber.subscribed
 "You have unsubscribed from notifications. Test 'TULIP' to start receieving updates again"
 else
 "Sorry, we don't recognize that command. Available commands are: 'TULIP' or 'STEM'."
 end
end
def standard_groups
 %w(dancers staff volunteer)
end

I was able to remove the output variable by leaving the string that you want to return as the last line that will run within the method. The exception is the line 'returned' if subscriber.subscribed comes back true.

I moved the common messages to an array so that you can easily add more in the future.

I considered separating the conditional for 'tulip' and 'stem' into 2 individual conditionals, but to me it made more sense to keep the "exceptions" together, especially since they share the 'visitor' group. For increased legibility, you may choose to separate them.

• Phrancis: Code-only answers are not likely to be very helpful to the asker, as well as future readers of your answer. Please see How do I write a good answer?
• MTarantini: I see what you mean, thanks for the feedback. Hopefully my edits will improve the answer.

Answer 2

Cleaning up your logic a bit yields this:

def process_message(message, subscriber)
 case message
 when "tulup"
 subscriber.update(subscribe: true)
 subscriber.add_to_group("visitor")
 "You are now subscribed for updates."
 when "stem"
 subscriber.update(subscribed: false)
 subscriber.add_to_group("visitor")
 "You have unsubscribed from notifications. Test 'TULIP' to start receiving updates again"
 when "dancers"
 subscriber.update(subscribed: true)
 subscriber.add_to_group("dancers")
 "You have been subscribed to the Dancers list"
 when "staff"
 subscriber.update(subscribed: true)
 subscriber.add_to_group("staff")
 "You have been subscribed to the Staff list"
 when "volunteer"
 subscriber.update(subscribed: true)
 subscriber.add_to_group("volunteer")
 "You have been subscribed to the Volunteer list"
 else
 # If we don't recognize the command, text back with the list of
 # available commands
 "Sorry, we don't recognize that command. Available commands are: 'TULIP' or 'STEM'."
 end
end

You can see that I transformed your if expression into a case. I've also separated "tulip" and "stem" cases into separate cases. As far as I could tell the only reason you combined them into one is that they both called subscriber.add_to_group("visitor"). One line's duplication is easily justified by the improved clarity.

Now we can see where the duplication lies. Each branch in the case expression does three things: 1. Updates the subscriber's subscribed attribute according to message; 2. Adds the subscriber to a group according to message; and 3. Returns some output according to message.

I said "according to message" three times just now to emphasize that in each case we have a mapping from message to some value: message to group, message to subscribed, and message to output. I'll come back to subscribed, but for the other two we see a clear opportunity to use a hash:

COMMAND_TO_GROUP = {
 tulip: "visitor",
 stem: "visitor",
 dancers: "dancers",
 staff: "staff",
 volunteer: "volunteer"
}.freeze
COMMAND_TO_OUTPUT = {
 tulip: "You are now subscribed for updates.",
 stem: "You have unsubscribed from notifications. Test 'TULIP' to start receiving updates again",
 dancers: "You have been subscribed to the Dancers list",
 staff: "You have been subscribed to the Staff list",
 volunteer: "You have been subscribed to the Volunteer list"
}.freeze

Now for each message we can just do a hash lookup to get the appropriate group and output.

As for subscribed, we only have one false case ("stem"), so a hash might be overkill. But it's still good to move that logic into a method:

VALID_COMMANDS = Set.new(%i[ tulip stem dancers staff volunteer ]).freeze
def update_subscribed!(subscriber, command)
 raise "Invalid command!" unless VALID_COMMANDS.member?(command)
 subscribe = !(command == :stem)
 subscriber.update(subscribe: subscribe)
end

Now we have all the pieces we need to refactor the process_message method:

def process_message(command, subscriber)
 command = command.downcase.to_sym
 unless VALID_COMMANDS.member?(command)
 return "Sorry, we don't recognize that command. Available commands are: " \
 "'TULIP' or 'STEM'."
 end
 update_subscribed!(subscriber, command)
 subscriber.add_to_group(COMMAND_TO_GROUP[command])
 COMMAND_TO_OUTPUT[command]
end

I've put the complete code at the end of this answer.

A few more important notes:

1. This shouldn't be in your controller. Logic like this should go in a model or a service object.

2. A smarter place to put the strings that I've put in the COMMAND_TO_OUTPUT hash would be in locale files, e.g.:

   # config/locales/en.yml
   en:
    command_response:
    tulip: You are now subscribed for updates.
    stem:
    You have unsubscribed from notifications. Test 'TULIP' to start receiving
    updates again.
    # ...
   

   Then you could fetch the messages using the I18n API, e.g. I18n.t('command_response.#{command}').

---

Complete code:

VALID_COMMANDS = Set.new(%i[ tulip stem dancers staff volunteer ]).freeze
COMMAND_TO_GROUP = {
 tulip: "visitor",
 stem: "visitor",
 dancers: "dancers",
 staff: "staff",
 volunteer: "volunteer"
}.freeze
COMMAND_TO_OUTPUT = {
 tulip: "You are now subscribed for updates.",
 stem: "You have unsubscribed from notifications. Test 'TULIP' to start receiving updates again",
 dancers: "You have been subscribed to the Dancers list",
 staff: "You have been subscribed to the Staff list",
 volunteer: "You have been subscribed to the Volunteer list"
}.freeze
def process_message(command, subscriber)
 command = command.downcase.to_sym
 unless VALID_COMMANDS.member?(command)
 return "Sorry, we don't recognize that command. Available commands are: " \
 "'TULIP' or 'STEM'."
 end
 update_subscribed!(subscriber, command)
 subscriber.add_to_group(COMMAND_TO_GROUP[command])
 COMMAND_TO_OUTPUT[command]
end
def update_subscribed!(subscriber, command)
 raise "Invalid command!" unless VALID_COMMANDS.member?(command)
 subscribe = !(command == :stem)
 subscriber.update(subscribe: subscribe)
end