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:
after_savecallback will raise an exception.
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
API_KEY or a
LIST_ID 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
LIST_ID from? It doesn't really matter, as long as it's not in the
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.
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.