Ruby on Rails: Clean and Thin Controllers

There are many articles, blog posts, guides, books, poems and urban legends on how to keep your Rails controllers clean and DRY. 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. As it seems everyone knows what a thin controller is, but when looking into open source projects or doing internal code reviews we all continue to encounter controllers that are neither thin, nor clean, nor simple, nor testable.

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

Our Business Case

Let’s play a game. If you have ever played a MOBA game, like LoL or Dota or HotS, you know the arena preparation where players choose heroes from a pool of available heroes. A player has about 30 seconds to choose a hero to control during the game. In this time frame he can select and deselect a hero as many times as he wants until his decision is final or the time bank expires. When a player picks a hero that hero is removed from the arena pool and assigned to the player. When a player deselects a hero that hero is returned to the arena pool and unassigned from the player.

Our Technical Approach

The head first approach would be to add two new methods to our ArenasController beautifully named select_hero and deselect_hero and call it a day. But since we are experienced Rails programmers trying to write good code, 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. Which can be things like filters. Like, say, you have this screen and it looks in a certain state. Well, if you apply a couple of filters and dropdowns to it, it’s in a different state. Sometimes we just take those states and we make a brand new controller for it. 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 wisdom into account below is our beautifully crafted code written with so much care and passion. The create method selects a hero for a player and the destroy method deselects a hero. The code is adherent to the REST principles and completely in sync with DHH advice.

Our Solution

class HeroSelectingController
  def create
    arena = Arena.find params[:arena_id]
    hero = Hero.find params[:id]

    current_user.hero = hero
    arena.pool.heroes.delete hero

    redirect_to arena, notice: "Hero #{hero.name} selected"
  end

  def destroy
    arena = Arena.find params[:arena_id]
    hero = Hero.find params[:id]

    current_user.hero = nil
    arena.pool.heroes << hero

    redirect_to arena, notice: "Hero #{hero.name} recalled"
  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 (omitted here for conciseness) are green. But still something is not quite right. 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 just sent into oblivion. 1 Mississippi, 2 Mississippi, 3 Mississippi… 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 far from DRY. But that’s easy to work out. Let’s use the extract method pattern to DRY the repeated code out. The finding of a hero by ID is extracted into a private method of the controller. The hero object is memoized to avoid hitting the Rails query cache. The same goes for the arena object.

class HeroSelectingController
  def create
    current_user.hero = hero
    arena.pool.heroes.delete hero

    redirect_to arena, notice: "Hero #{hero.name} selected"
  end

  def destroy
    current_user.hero = nil
    arena.pool.heroes << hero

    redirect_to arena, notice: "Hero #{hero.name} recalled"
  end

  private

  def arena
    @arena ||= Arena.find params[:arena_id]
  end

  def hero
    @hero ||= Hero.find params[:id]
  end
end

Goooood! Our controller is DRY now and reads easily almost 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. 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 in time 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.


This Code Will Not Tolerate Change

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

  • how hero is assigned to user
  • how hero is unassigned from user
  • how hero is added to arena’s pool
  • how hero is removed from arena’s pool
  • the arena to pool relation
  • the user to hero relation

then you will have to come to this controller and change it as well. These are many dependencies and far too many reasons to change that small piece of code.

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. That’s what we refer to as a spaghetti code. Arrows pointing out and in from every object to an arbitrary amount of other objects. It’s seems like too much theory and one might ask: “Why should I care? It’s there and it works”. Believe me - you should care a lot. The problem with writing bad code comes at a later stage in the application development. In the beginning everything is working great, the team adds feature after feature at a good speed, the product owners are happy, the clients are happy, life is beautiful. But after the code base have grown enough, you end up working with a big ball of mud in which dependencies are so mixed up that a change in one place requires at least several other changes in several other places. It looks something like the picture on the right:

this

Your team velocity will decrease by a factor. No one can be efficient when working in such a mess.

Minimize the dependencies in your code.

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 lot of setup and many asserts. That should ring some bells. It is hard to test our controller because it is not clean and simple. The object under test has way too many dependencies. To test your object you have to stub all its collaborators. So you are either testing nothing because you have stubbed and mocked everything, or you are not having isolated unit tests but a bunch of failing integration tests which are so tightly coupled with your code that every time you have to make a change in your code you have to change also all the tests that depend on that code. That’s why people advocate for Test-driven Development (TDD). TDD will slap you across the face if your design is bad. Start with how your code will be used in mind. Eat your own dog food. 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. The first letter stands for Single Responsibility Principle (SRP). That is yet another principal that we broke. 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 six reasons to change our controller besides client requirements. It knows too much about “selecting a user”. It knows about the behavior and the internal structure of three other classes. It knows how the User model selects and deselects a hero internally. It knows that the Arena model has a Pool, and that the pool object keeps all the heroes in a list and knows how to manage that list directly. These are way too many responsibilities for a single class. Whenever you decide to change something in the User or Arena or Pool objects you will have to come to this controller and adapt the code to the new changes. And that’s way too far from single responsibility.

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 arena object is our immediate collaborator. We can talk to it to add or remove heroes. But knowing that it has a pool collaborator and that pool manages all the heroes as a list, and doing operations on that list is one step too far. Now we have coupled the controller to the arena’s pool’s list of heroes. If at some point we decide to store those heroes not as a list but as a map, we will have to go to every object that is coupled to that list and apply the change also there. Breaking the LoD principle couples objects tightly together throughout the dependency chain. It reveals your collaborators guts and makes testing a real pain.

No Business Logic in the Controller

The controller should not have any logic inside. It should make one single call to a model or a service and receive one single object from that call. It should only control the overall process dealing mainly with request and response handling. It should house no business logic or knowledge about its collaborator’s internal structure and behaviour. Looking back at our code sample you can see that the controller knows how to assign a hero to the user via the hero association, i.e it knows the internal structure of the User class. It knows how to remove a hero from the arena pool of heroes. It knows about the guts of the Arena class, i.e. how to access and work with the heroes inside the pool. All of that should be refactored out of the controller.

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

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

Our Refactored Solution

class User
  def select_hero(hero)
    self.hero = hero
  end

  def deselect_hero
    self.hero = nil
  end
end

class Pool
  def add(hero)
    heroes << hero
  end

  def remove(hero)
    heroes.delete hero
  end
end

class Arena
  delegate :add, to: :pool
  delegate :remove, to: :pool
end

class HeroSelector
  def initialize(arena, user)
    @arena = arena
    @user = user
  end

  def run(hero)
    @user.select_hero hero
    @arena.remove hero
  end
end

class HeroDeselector
  def initialize(arena, user)
    @arena = arena
    @user = user
  end

  def run(hero)
    @user.deselect_hero
    @arena.add hero
  end
end

class HeroSelectingController
  def create
    HeroSelector.new(arena, current_user).run hero
    redirect_to arena, notice: "Hero #{hero.name} selected"
  end

  def destroy
    HeroDeselector.new(arena, current_user).run hero
    redirect_to arena, notice: "Hero #{hero.name} recalled"
  end

  private

  def arena
    @arena ||= Arena.find params[:arena_id]
  end

  def hero
    @hero ||= Hero.find params[:id]
  end
end

Let’s analyze our final solution. We have factored out the business logic into separate abstractions. Now the controller does not handle any business logic directly. Each controller action calls out 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 - hero selection and dismissal. The abstractions are black boxes. The controller is completely ignorant of their state, 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 business logic from the controller 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. In short: nobody knows about the guts of anybody.

The controller depends only on the SelectHero and DismissHero services. It does not know what they do or how they do it. If you need to change something about that feature you change it in the corresponding place only without affecting other places in our code. If you need to change the internal structure or behavior of its collaborators (Arena, Pool or User), then you can do that inside those classes without affecting the services. For example, you can change the array of heroes to become a hash of heroes. That change will happen only inside the Pool class and the Arena will never know, as well as the controller will never know. The dependents do not need to be adapted to that change as they collaborate through clear interfaces without mixing their internals like spaghetti. The SelectHero does not know how to remove a hero from the pool. It delegates that task to the Arena which in turn delegates it to the Pool. The DismissHero does not know how to clear a hero from the user. It delegates that task to the User model. All the responsibilities are isolated into their own abstractions. The User and Arena dependencies are injected into the SelectHero and DismissHero so we can easily pass any object in there when writing our unit tests. You see how good code results in good tests. At this point you would barely need any controller tests as all business logic will be tested in the SelectHero and DismissHero specs.

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