Ruby on Rails: Clean and Thin Controllers

Clean Code Ruby on Rails

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 user story: 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 on the business rules. Users can subscribe to forum discussions in order to receive notifications about new posts and comments. The moment the user subscribes or unsubscribes they should receive a subscription changed notification email. As simple as it could ever be.

Our approach

The database schema should be obvious. 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 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

To keep the examples small and simple I would skip all cross-cutting concerns like migrations, security, validations, etc. Let’s focus on the fundamental principles that guide us when writing good 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. As way too often I get “Keep the classes and methods small and that’s all” on my question “How to write good code?”.

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, the tests are green. Even DHH could say that it has “clarity” so it’s fine. But still something smells. Stop here for a moment and think about what is wrong with our code.

Don’t Repeat Yourself

Obviously our controller is not DRY. I put DRY first here not because it is the most important thing, but because it is the easiest to refactor. In general, I am not too concerned when things are not DRY. The repetitiveness here may not even classify as true code duplication as people may argue that those pieces may have different reasons to change as subscribe is the exact opposite of unsubscribe. Anyways, let’s not get stuck in analysis paralysis, but clean this up a bit. 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.

Action callbacks are evil

Another approach here could be to use a before_action callback, like Rails scaffolding would do for fetching that discussion by ID. My preference is that callbacks shouldn’t hold anything even remotely classified as a business rule. They are so implicit. They do their work secretly, behind the scenes, in the dark. Having a bunch of them and it will be impossible to follow along your code. Variables appearing from the dark into the Rails global state out of nowhere conditionally based on which action method you are in. Sounds like a horror movie. I feel scared already. My preference is that callbacks are reserved for horizontal concerns like authentication, authorization, logging, etc. And that’s it, no more. You draw a hard thick line right there and don’t let anything cross that boundary ever. But enough about callbacks and let’s get back to 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.

MVC is not an architecture

But is this a good structure? Is this a good architecture? If you are doing things the Rails way, you should first throw out the window all the good practices on doing architecture right and then embrace the MVC as the solution to all your problems. Well, that could work if you are writing a simple todo application or a blog. But that would cause you a lot of pain when working on a large monolitic application. When working on a complex application having many business rules, having multiple delivery mechanisms (ex. AMQP and HTTP), having multiple data stores, only then you can feel the real pain of having the wrong architecture, of having the wrong abstractions, of having coupled your controllers to your models to your domain logic.

I have a good example to support all that from a recent project at work. We have that application that was communicating with the outside world using a message broker. A some point we decided to rework this application as a web service that communicates over HTTP. We did this in a couple of days given it was a relatively large application. How? Because all the domain logic was isolated from all the low-level details. Yes, Rails controllers are a low-level detail. They are not important. They are just a delivery mechanism on how you receive your input. Yes, your database, the one and only thing toward which all your code point to, is a low-level detail. It is not important. It is just a storage device. These could be easily replaced if the need changes. They are a pluggable moving parts of your architecture. But that’s all true only if your whole architecture is not based on MVC.

Is good architecture worth the effort?

Is the value from having the right structure greater than the cost? Do you buy it? Should we walk instead of run? Good architecture and good code require extra effort. You may not sprint when doing things right. You have to do some extra work to get the structure right. That extra work pays off in a large application. That extra work doesn’t pay off in a small application. So, should you do it? It depends. It depends on the size and complexity of your application. It depends on your business domain - are you writing banking software or a blog? It depends on your teams structure. It depends on the clients of your application. Is your application called via HTTP only or it has a CLI as well. The list goes on and on.

Take for example AWS. You can spin off a EC2 instance using the CLI, using the REST API, and using the UI in the browser. Imagine the reaction of the team doing the command line interface, who wants to reuse the domain logic around spinning a new EC2 instance, if the team doing the web part has put all their code in the controller. Imagine the WTFs per minute when reading that controller code. Keep those controllers thin, guys. Remember, they are just a delivery mechanism. They are not important.

Let’s get back to our “work” again, and sorry for all the distraction, but I felt it is necessary to bring some perspective on why in the world we are doing that extra effort to get out of those MVC boxes. The TL;DR answer - they are not enough. We need more boxes to do things right.

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 all the way through. It should stay focused on “what”, not on “how”. It should depend on an abstraction. It should depend on behaviour. It should send a message to an object along with some input, get the result, and deal with it. That’s all a controller should be responsible for. That should be the only reason you will ever have to make a change in a controller - low-level HTTP details. If the business logic changes, and you have to go to a controller and change it - guess what, you are doing it wrong.

The controller is not important. Keeping things loosely coupled is important. You want to have things loosely coupled so that you can change them without affecting each other, so that you can reuse them, so that you can test them. The implementation details around subscriptions could change and you still want everything to work correctly across your application. If you extract this logic out of your controller you may reuse it in another place. You want to test your business rules separated from the input delivery mechanism. Why? Because you don’t care how the input reaches your business logic. It’s not important.

Changeable code

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 dependencies

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 test this controller. 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

Try to test your business rules. You will have to spin off the whole world to test everything through the controller. You will have to deal with HTTP in order to feed input to your business rules. A Rails advocate would say here - “That’s ok, we have fixtures. We will fix the whole world exactly the way you need it”. Well, it is not OK to test each and every use case your system may have through the UI. You may have one happy-path functional test per business rule that confirms everything still works end to end. But everything else, all those control paths in your code, should be tested by hitting your high-level abstractions directly, oblivious of HTTP, the web, and the world around you. Oblivious of the low-level details. Imagine working on a large application and testing every use case through the UI. It will take hours before you validate your change.

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. And using a dynamic language is the only reason we didn’t break the last one. 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 rules 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.

Takeaway

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