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

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.

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