Ruby on Rails: Clean and Thin Controllers

There are many articles, blogs, guides, books, poems and urban legends on how to keep your Rails controllers clean and simple. At job interviews that very question is being asked almost every time. Because of all that fuss around thin controllers even junior programmers without much of a practical Rails experience know THE answer. It seems everyone knows what a thin controller should look like but when peeking into open source projects or doing code reviews we all continue to see controllers that are neither thin, nor clean, nor simple, nor testable. They are just doing too much and the logic in there cannot be reused elsewhere.

How is that even possible? Hmm… meditate on that I will.

Our use case: Subscribe to a discussion

Let’s take an obvious example everybody is familiar with so that we would focus on the technical approach rather than elaborating on the requirements of a complex business process. Users can subscribe to forum discussions in order to receive notifications about new posts and comments. The moment the user subscribes or unsubscribes he or she should receive a subscription changed notification email.

Our technical approach

The database design is straightforward. We associate users to discussions in a many-to-many relationship through a join table called subscriptions.

When defining the routes a head first approach would be to add two new methods to our DiscussionsController named subscribe and unsubscribe and call it a day. But since we are good Rails citizens trying to keep our routes adherent to the REST principles, we will draw a new controller instead, following the guidance of our Lord and Savior DHH in his interview for the Full Stack Radio. As the radio cast is quite lengthy, I will quote only the point we are taking out of it for our decision to spin off a new controller:

What I’ve come to embrace is that being almost fundamentalistic about when I create a new controller to stay adherent to REST has served me better every single time. Every single time I’ve regretted the state of my controllers, it’s been because I’ve had too few of them. I’ve been trying to overload things too heavily. So, in Basecamp 3 we spin off controllers every single time there’s even sort of a subresource that makes sense. The heuristics I use to drive that is: whenever I have the inclination that I want to add a method on a controller that’s not part of the default five or whatever REST actions that we have by default, make a new controller! And just call it that.

Taking all that guidance into account below is our beautifully crafted code written with so much care and passion. The create method subscribes a user to a discussion, i.e. creates a subscription. The destroy method unsubscribes a user from a discussion, i.e. destroys a subscription. The code is adherent to the REST principles and completely in sync with DHH advice.

Our Solution

Disclaimer: to keep the examples small and easy to understand I would skip all cross-cutting concerns like migrations, security, validations, etc. Let’s focus on the fundamental principles that guide us when crafting clean code. The code is intentionally very very short and simple to emphasize on the fact that small classes and methods alone do not make your abstractions clean.

class Subscription < ApplicationRecord
  belongs_to :discussion
  belongs_to :user
end

class Discussion < ApplicationRecord
  has_many :subscriptions
  has_many :users, through: :subscriptions
end

class User < ApplicationRecord
  has_many :subscriptions
  has_many :discussions, through: :subscriptions
end

class SubscriptionsController < ApplicationController
  def create
    discussion = Discussion.find params[:discussion_id]

    current_user.subscriptions.create! discussion: discussion

    UserMailer.with(discussion: discussion, user: current_user).subscribed.deliver_later

    redirect_to discussion, notice: "Subscribed."
  end

  def destroy
    discussion = Discussion.find params[:discussion_id]

    current_user.discussions.delete discussion

    UserMailer.with(discussion: discussion, user: current_user).unsubscribed.deliver_later

    redirect_to discussion, notice: "Unsubscribed."
  end
end

Piece of cake. It seems that all the coding Gods are with us and we can go grab some beers :)

But Are We Done Yet?

I don’t know for you but I don’t like that code very much. Ok, it works. Ok, it does the job. Ok, the tests are green. But still something smells. Stop here for a moment and think about what is wrong with our code. Try to come up with at least three fundamental principles that we have sent into oblivion. Ok, time’s up! Let’s now count how many software development principles we have broken with our otherwise so small and beautifully crafted code.

Don’t Repeat Yourself

Obviously our controller is not DRY. But that’s easy to work out. Let’s use the extract method pattern to DRY the repeated code out. The discussion object is memoized to avoid hitting the Rails query cache.

class SubscriptionsController < ApplicationController
  def create
    current_user.subscriptions.create! discussion: discussion

    mailer.subscribed.deliver_later

    redirect_to discussion, notice: "Subscribed."
  end

  def destroy
    current_user.discussions.delete discussion

    mailer.unsubscribed.deliver_later

    redirect_to discussion, notice: "Unsubscribed."
  end

  private

  def discussion
    @_discussion ||= Discussion.find params[:discussion_id]
  end

  def mailer
    UserMailer.with discussion: discussion, user: current_user
  end
end

Good. Our controller is DRY now and the actions read easily in plain English. Another approach here could be to use a before_action callback, but I really do not like callbacks to create hidden instance variables behind the scenes. It is so implicit. I’d rather use private methods as those add that Ruby awesomeness where bare words can be used to nicely describe what we are doing. At this point our code may look quite well on paper and even pass code reviews in a significantly large number of Rails software houses but we know it is far from perfect so we shall continue our countdown of broken principles.

Tight coupling

We have coupled our controller to the actual implementation of how a user can subscribe to a discussion. The controller knows about the database associations and the concrete operations that need to be performed for subscribing a user to a discussion. We want this imperative approach to be more declarative. The controller should declare what it needs instead of knowing how to accomplish it. It should stay focused on “what”, not on “how”.

First, keeping things loosely coupled is very important because the implementation details around subscriptions could change later on and you still want everything to work correctly across your application. Second, if you extract this logic out of your controller you may reuse it in another place.

This code will not tolerate change

Every time we have to change something due to changes in the requirements or changes in the database models, that change in that “thing” will propagate changes in all other “things” that depend on it cascadingly. In our solution, if any of these changes:

  • subscription process
  • discussions to users relationship
  • notification preferences

Then we will have to come to this controller and make changes. Open-closed principle - fail. Single responsibility principle - fail. The controller is not closed for modification and there are far too many reasons to change that small piece of code.

Minimize the dependencies in your code

Our controller depends on three database models and a mailer object. That may not be obvious to the naked eye without a test to explicitly show all the setup and stubbing, but these dependencies sit there quietly waiting to slap you across the face at the next change of requirements.

When you refer to something, you depend on it. When the things you depend on change, you must change. - Sandy Metz

When things are tangled tightly with each other and you have to change something then you will have to change all its dependents. Follows an artist impression of dependency hell.

Bad code results in bad tests

Try to write controller tests for our solution. No matter what approach you take you will have to do a complicated fixture and multiple asserts. That should ring some bells. It is hard to test our controller because it knows too much. The object under test has many collaborators. And guess what, once the code changes you will have to go down here to these specs and adapt them as well.

Seasoned developers advocate for Test-driven Development (TDD). TDD will give you early feedback about wrong designs and will keep you focused on “what” to achieve, not on “how”. Remember: well-factored code is easy to test.

If testing seems hard, then there is a problem with your design. - Sandy Metz

Single Responsibility Principle

You probably have heard about SOLID principles: single responsibility, open-closed, liskov substitution, interface segregation and dependency inversion. Well, with our beautiful solution we just broke the first two of them. Let’s focus on Single Responsibility Principle (SRP). The principle states that an object should not have to change for more than one reason. The object should have a single responsibility so that you do not have a lot of reasons to modify its code. You should create classes that do one thing. Above we have counted three good reasons to change our controller. It knows too much about the subscription process. It knows about the database structure of the models. It knows all implementation details about how a user is subscribed to a discussion. It knows that part of this process is notifying the user by email for this action. At least the controller delegates this task to the mailer without knowing how to actually send an email message. These are way too many responsibilities for a single class.

Law of Demeter (Principle of Least Knowledge)

The Law of Demeter (LoD) or principle of least knowledge is an object-oriented design guideline that is a specific case of loose coupling. It states that each object should only talk to its immediate collaborators and not talk to the collaborators of its collaborators. Only talk to your immediate friends. Do not talk to your friend’s friends. Otherwise you’d end up in a dependency hell. In our controller the User object is our immediate collaborator. We can talk to it to subscribe to a discussion, for example like this:

current_user.subscribe_to discussion

However, this is not our current solution. In our current solution, the controller knows that user has a relation to a subscription and that the subscription belongs to a discussion. Now we have coupled the controller to the whole relationship chain. If one link in that chain changes we will have to go to every object that is coupled to it and adapt to that change. Breaking the LoD principle couples objects tightly together throughout the dependency chain. It reveals your collaborators guts and makes testing a real pain.

No domain logic in the controller

Business logic does not belong to the controller. A controller should make one single call to a model or a service and receive one single object from that call. The controller is a thin layer between the HTTP request/response and the application logic. It should handle the request parameters, invoke the service layer and build a response (redirect or render). In short: it should deal with the transport-related stuff and delegate everything else. In our example the whole subscription process should be refactored out of the controller.

Take away: The controller should know what it needs but does not know how to get it.

Our Refactored Solution

A good way to refactor our code would be to use the extract class pattern and extract all the domain logic into a new abstractions that falls under the SOLID principles. When creating new abstractions focus on using nice clean interfaces and simple classes with obvious responsibilities.

class User < ApplicationRecord
  def subscribe_to(discussion)
    subscriptions.create! discussion: discussion
  end

  def unsubscribe_from(discussion)
    discussions.delete discussion
  end
end

module DiscussionSubscriber
  extend self

  def execute(user, discussion)
    user.subscribe_to discussion
    UserMailer.with(discussion: discussion, user: user).subscribed.deliver_later
  end
end

module DiscussionUnsubscriber
  extend self

  def execute(user, discussion)
    user.unsubscribe_from discussion
    UserMailer.with(discussion: discussion, user: user).unsubscribed.deliver_later
  end
end

class SubscriptionsController < ApplicationController
  def create
    DiscussionSubscriber.execute current_user, discussion
    redirect_to discussion, notice: "Subscribed."
  end

  def destroy
    DiscussionUnsubscriber.execute current_user, discussion
    redirect_to discussion, notice: "Unsubscribed."
  end

  private

  def discussion
    @_discussion ||= Discussion.find params[:discussion_id]
  end
end

Let’s analyze our final solution. We have factored out the domain logic into separate abstractions. Now the controller does not execute any business logic directly. Each controller action delegates to a service object to do the heavy lifting for it. Each action makes one call to one abstraction which is better than having a single abstraction doing two things - subscribing and unsubscribing. The abstractions are black boxes. The controller is completely ignorant of their behavior and collaborators. The controller does not know how to do stuff. It only knows what needs to be done and where to go after it’s done. We have decoupled our application logic from the transport flow. But not only that, we have decoupled all of our objects from each other and placed each responsibility into its own abstraction. Now all classes can change internally without affecting other places of our code. You will rarely need to change the controller code because all the business logic is isolated out of it into the right components. That logic can now be easily tested without the whole HTTP layer.

But Original Code Was Shorter

If I got a penny every time I hear that out, I would have become a billionaire by now. You will always get that call from at least one person within your team. It is the inevitable doom. My answer to that was and will always be the same:

If you only gonna have one job ever in your life then protected it by all means. Even by writing spaghetti code that no one else will be able to extend or maintain. But if you want the project to be a success then start writing clean code that you can easily change. Working with well-factored abstractions that are intuitive and easy to use is so much fun. Do not resist it - just learn to love it. You will do a great favor to your future self and your team when the project grows bigger.

Now we can call it a day. Thank you for bearing with me up to now. I really hope you have learned something new or refreshed something old. Right on time to go grab those beers!

References