Don't just dump code into your modelsPublished on Apr 19, 2014

A few years ago there used to be a motto saying Skinny controllers, fat models. It was good at the time, because people thought all of the application code belonged into the controllers, and it helped them realize that it’s good to have just a simple controller layer and push things down. The problem with this motto is that it’s wrong. Just because you should have skinny controllers it doesn’t mean you should have fat models. In fact, none of your classes should be fat.

The reason we have MVC is that it is a good starting point for separating the concerns in your application. It makes it more obvious where you should put your code when you’re starting out with your application. But this doesn’t mean your application should be limited to just these three layers, quite the opposite.

Every time you need to add a new service to your application, you should think about how you’re going to be using it, and then build a sufficient abstraction on top of that.

For example let’s say that your application needs to automatically sign up newly registered users to a mailing list. You’ve even chosen a provider, say AWeber or Mailchimp, as both of these have a nice Ruby gem wrapping up their API, allowing you to subscribe the user with only a few lines of code.

Here’s how the naive solution might look like

class User
  after_create :subscribe_to_mailchimp

  def subscribe_to_mailchimp
    list_id = ENV["MAILCHIMP_LIST_ID"]
    Mailchimp.new(ENV["MAILCHIMP_API_KEY"].subscribe(list_id, self.email)
  end
end

If you happen to like YAML files more than environment variables, you might write something like this instead

def subscribe_to_mailchimp
  config = SomeGlobalYamlConfigReader.mailchimp
  Mailchimp.new(config["api_key"].subscribe(config["list_id"], self.email)
end

While this might make sense the first time you try this, there are actually many things wrong with this approach. Let’s list a few:

If the Mailchimp API is unavailable during the time you decide to create a new user, your after_save callback will raise an exception.
If an exception is raised, ActiveRecord will rollback the transaction and your user won’t get created.
Even if you choose to handle the exception in the callback, you’ll fail to subscribe the user to the mailing list.
Your User model becomes coupled with the Mailchimp gem. If for some reason you later decide to switch to AWeber, you’ll have much harder time doing so (probably because you have Mailchimp in more than one place).

These are just a few problems. There are many more, for example you probably don’t even want to run this code in development/test environment, which means the method should be changed to something like

def subscribe_to_mailchimp
  if Rails.env.production?
    config = SomeGlobalYamlConfigReader.mailchimp
    Mailchimp.new(config["api_key"].subscribe(config["list_id"], self.email)
  end
end

but this is still wrong, because you might have a staging server where you do want to run this callback, making the method even more complicated.

But wait, there’s more. Let’s say you magically manage to overcome all the above issues and actually decide to test this method. A thought pops up in your head and you think I should probably be able to handle if I get a bad `APIKEYor aLISTID` which doesn’t exist on Mailchimp. But you have no easy way of testing that, because your callback is tightly coupled to the global configuration (or ENV variables). Wouldn’t it be much nicer if you could just inject the API_KEY and the LIST_ID somehow? Maybe even because you later decide that you need to subscribe different users to different mailing lists.

Here’s a little less wrong approach

# notice there's no mailchimp in the name
class EmailMarketing::Subscription
  attr_reader :api_key

  def initialize(api_key)
    @api_key = api_key
  end

  def subscribe(user, list_id)
    Mailchimp.new(api_key).subscribe(list_id, user.email)
  end
end

# now whenever you need to subscribe a new user, you'd just write
EmailMarketing::Subscription.new(API_KEY).subscribe(user, LIST_ID)

Where do you get the API_KEY and LIST_ID from? It doesn’t really matter, as long as it’s not in the Subscription or User classes that fetch it. Every class should have a single reason to change, and if you need to change your User model because you changed the way you store your email marketing API key, you’re doing it wrong.

Where do you put this class? Again, it doesn’t really matter. Maybe something like lib/email_marketing/subscription might make sense. When you later decide to add lib/email_marketing/campaign, it won’t look as stupid as having all that code in the models.

Why isn’t subscribe a class method? There are many reasons why you should use instance methods and save yourself some trouble.

Because of the problems mentioned earlier, the only way to solve this issue correctly is to use a background queue such as Sidekiq or Resque. There is one thing that’s always true for interacting with 3rd party services, they will fail occasionally. It’s not a question of if, it’s a question of when, and you have to be ready for it.

It’s completely normal that a service goes down for a few seconds or even minutes every now and then. If you have the subscription logic in your controller when the user is signing up, he might end up registering an account but because the request to Mailchimp fails he actually won’t get added to the mailing list.

This is why you have to use a background service that will try to add the user to the list, and if that fails it will automatically re-schedule a retry.

class UsersController < ApplicationController
  def create
    @user = User.new(user_params)
    if @user.save
      EmailSubscriptionWorker.perform_async(@user.id)
      redirect_to root_path, notice: "Welcome!"
    else
      render :new
    end
  end
end

class EmailSubscriptionWorker
  include Sidekiq::Worker

  def perform(user_id)
    api_key, list_id = # read this from some config

    user = User.find(user_id)
    EmailMarketing::Subscription.new(api_key).subscribe(user.email, list_id)
  end
end

You could also have a cron job that periodically checks if all users are signed up for the service, as an alternative to the background worker, but using a queue is a far superior solution.

Written by Jakub Arnold of sensible.io.