Skinny controller, fat model – the old advice

In 2006 Jamis Buck wrote a famous post called Skinny Controller, Fat Model. In it Jamis observed that Rails developers often put too much logic in controllers, making the code harder to understand and to test than it needs to be.

“Skinny controllers, fat models” became a piece of accepted wisdom in the Rails community. It was a step forward in the overall state of affairs, but a problem remained: moving code out of controllers and into models often resulted in fat models.

The new advice: skinny everything

Over time developers apparently came to a realization that while it’s better in certain ways to put bulky code in models than in controllers, the fatness, wherever you put it, is still kind of a problem.

Other ways of combating digital obesity were adopted or devised, including concerns, service objects, domain objects, Interactors, factories, something called Trailblazer, and probably others. These solutions vary in how good they are and where they’re appropriate to use.

Not all weight loss methods are equally good

When we talk about making e.g. a controller less fat, we of course aren’t talking about obliterating the bulk from existence but rather just moving it to a more appropriate place. The world is complex and we can’t make the complexity go away. The art is in distributing the complexity throughout the code such that a human can understand what the application does, in spite of the complexity.

For whatever reason, it seems lately that “service objects” (a term which means different things to different people, hence the quotes) and Interactors are widely esteemed as great ways to reorganize complexity in a Rails application. I personally find Interactors and service objects unappealing. In fact, I think they’re terrible. Avdi Grimm seems to have a similar opinion.

I think service objects and Interactors add unnecessary complexity to an application without adding any meaning to justify their added complexity. To take every action and make an object named after it—e.g. AuthenticateUser , PlaceOrder , SendThankYou —seems like a superfluous, tautological extra step. I think we can do better.

The case for POROs and domain objects

In Domain Driven Design, Eric Evans describes the concept of domain objects, which I take to mean: objects that represent concepts from the domain.

To me this makes a lot of sense. Most Rails applications of course already make use of this concept. If there’s something called an appointment in the domain, there will be an Appointment class in the application. If there’s something called a Patient in the domain, there will be a Patient class in the application.

It’s also possible to invent objects that don’t exist in the domain, that only exist to make the code easier to understand. It took me personally a long time to realize this. Examples of object-oriented programming often include things like creating a Dog and a Cat class, each of which inherit from Animal . These examples seem sensible and straightforward, but think of how many objects in Ruby and Rails have nothing to do with any corresponding real-world idea, e.g. Request , MimeNegotiation and SingularAssociation .

These types of objects take a little extra imagination to conceive of and name, but I think once a person opens up their mind to this way of coding, it gets easier and easier.

The form that I like to give these sorts of objects is POROs (Plain Old Ruby Objects).

My example code: a messy Active Record class

For the remainder of this post I’m going to take a certain big, messy Active Record class and refactor it into some number of POROs.

The code below is taken from a real production application. It was written by a terrible programmer: me in 2012. The class represents a stylist at a hair salon.

I invite you to have a nice scroll through the code and give yourself a light familiarity with it.

class Stylist < ActiveRecord::Base include ActionView::Helpers::NumberHelper scope :active, -> { where(active: true) } scope :inactive, -> { where(active: false) } scope :with_active_salon, -> { joins(:salon).merge(Salon.active) } scope :non_receptionist, -> { joins('LEFT JOIN user_role ON user_role.user_id = stylist.user_id') .joins('LEFT JOIN role ON user_role.role_id = role.id') .where('role.code != ? OR role.code IS NULL', 'RECEPTIONIST') } belongs_to :salon belongs_to :user has_many :roles, through: :user belongs_to :stylist_employment_type has_many :stylist_services has_many :services, through: :stylist_services has_many :rent_payments has_many :appointments has_many :clients, through: :appointments attr_accessor :skip_saving_other_stylists accepts_nested_attributes_for :user, :allow_destroy => true, :reject_if => :all_blank after_initialize -> { self.skip_saving_other_stylists = false } before_save -> { make_sure_self_gets_correct_order_index } after_save -> { salon.reload.resave_stylists unless skip_saving_other_stylists } validates_presence_of :name validates_presence_of :salon validates_each :name do |model, attr, value| stylist = Stylist.find_by_name_and_salon_id(value, model.salon_id) if stylist != nil and stylist.id != model.id model.errors.add(attr, "A stylist with name \"#{value}\" already exists.") end end def service_length(service) ss = stylist_services.find_by_service_id(service.id) ss ? ss.length_in_minutes : "" end def service_price(service) ss = stylist_services.find_by_service_id(service.id) ss ? ss.price : "" end def unique_clients_ordered_by_name Client.select("client.*, TRIM(UPPER(client.name)) upper_name") .joins(:appointments) .where("appointment.stylist_id = ?", id) .uniq .order("upper_name") end def save_services(lengths, prices) stylist_services.destroy_all salon.services.active.each_with_index do |service, i| if lengths[i].to_i > 0 || prices[i].to_i > 0 StylistService.create!( stylist_id: self.id, service_id: service.id, length_in_minutes: lengths[i], price: prices[i] ) end end end def report(date) sql = " SELECT a.start_time, c.name client_name, ti.label item_label, CASE WHEN ti.is_taxed = true THEN ti.price * #{salon.tax_rate + 1} ELSE ti.price END FROM appointment a JOIN transaction_item ti ON ti.appointment_id = a.id JOIN client c ON a.client_id = c.id JOIN stylist s ON a.stylist_id = s.id WHERE s.id = #{self.id} AND a.start_time >= '#{Date.parse(date).strftime}' AND a.start_time <= '#{(Date.parse(date) + 7).strftime}' ORDER BY a.start_time, c.name, ti.label " result = self.connection.select_all(sql) result end def save_services_for_demo [{:name => "Men's Haircut", :price => 20, :length => 30}, {:name => "Women's Haircut", :price => 20, :length => 40}].each do |service| s = Service.create( :name => service[:name], :price => service[:price], :salon_id => self.salon_id ) StylistService.create( :service_id => s.id, :stylist_id => self.id, :length_in_minutes => service[:length] ) end end def formatted_rent if self.rent > 0 number_with_precision(self.rent, :precision => 2) else "" end end def formatted_service_commission_rate if self.service_commission_rate > 0 number_with_precision(self.service_commission_rate, :precision => 2) else "" end end def formatted_retail_commission_rate if self.retail_commission_rate > 0 number_with_precision(self.retail_commission_rate, :precision => 2) else "" end end def clean_values if self.rent == nil self.rent = 0 end if self.service_commission_rate = nil self.service_commission_rate = 0 end if self.retail_commission_rate = nil self.retail_commission_rate = 0 end end def pay_rent(date = Time.zone.now) payment = RentPayment.new payment.date = date payment.stylist_id = self.id payment.amount = self.rent payment.save end def rent_payments RentPayment.find_all_by_stylist_id(self.id) end def new_rent_payment rp = RentPayment.new rp.date = Time.zone.today rp.stylist_id = self.id rp.amount = self.rent rp end def gross_service_sales(start_date, end_date) self.salon.earnings(start_date, end_date, self.id)['services'].to_f end def net_service_sales(start_date, end_date) self.gross_service_sales(start_date, end_date) * self.service_commission_rate end def gross_product_sales(start_date, end_date) self.salon.earnings(start_date, end_date, self.id)['products'].to_f end def net_product_sales(start_date, end_date) self.gross_product_sales(start_date, end_date) * self.retail_commission_rate end def self.everyone_stylist self.new(id: 0, name: 'All Stylists') end def make_sure_self_gets_correct_order_index self.cached_order_index = order_index self.manual_order_index = manual_position if salon.has_manual_order? end def manual_position new_record? ? salon.highest_manual_order_index + 1 : manual_order_index end def alphabetical_position stylist_names = salon.ordered_stylist_names stylist_names << name unless stylist_names.member? name stylist_names.map(&:downcase).sort!.index(name.downcase) end def order_index return -1 unless active salon.has_manual_order? ? manual_position : alphabetical_position end def has_appointment_at?(start_time) appointments.where("start_time = ? and is_cancelled = false", start_time).any? end def stylist_employment_type_code stylist_employment_type ? stylist_employment_type.code : "" end end

The PORO to extract: calendar-related code

The class is so big and the code is so bad that it’s hard to decide where to start. I could spend quite a lot of time refactoring this class, but to make it all fit into a reasonable blog post, I’ll extract just one POROs out of this class.

There are a few methods that I happen to know are related to displaying the stylist’s name on the calendar:

def manual_position new_record? ? salon.highest_manual_order_index + 1 : manual_order_index end def alphabetical_position stylist_names = salon.ordered_stylist_names stylist_names << name unless stylist_names.member? name stylist_names.map(&:downcase).sort!.index(name.downcase) end def order_index return -1 unless active salon.has_manual_order? ? manual_position : alphabetical_position end

This is relatively easy low-hanging fruit. I can just conceive of a Calendar object and move these methods into it.

class Calendar def manual_position new_record? ? salon.highest_manual_order_index + 1 : manual_order_index end def alphabetical_position stylist_names = salon.ordered_stylist_names stylist_names << name unless stylist_names.member? name stylist_names.map(&:downcase).sort!.index(name.downcase) end def order_index return -1 unless active salon.has_manual_order? ? manual_position : alphabetical_position end end

This won’t exactly work, though. Many of the values referred to inside this new class only exist in the context of a Stylist . We’ll need to pass in a Stylist instance.

class Calendar def initialize(stylist) @stylist = stylist end def manual_position @stylist.new_record? ? salon.highest_manual_order_index + 1 : @stylist.manual_order_index end def alphabetical_position stylist_names = salon.ordered_stylist_names stylist_names << @stylist.name unless stylist_names.member? @stylist.name stylist_names.map(&:downcase).sort!.index(@stylist.name.downcase) end def order_index return -1 unless @stylist.active salon.has_manual_order? ? @stylist.manual_position : @stylist.alphabetical_position end private def salon @stylist.salon end end

Now that I see this much, I realize I’ve chosen an unfitting name for this class. All this code deals with figuring out where to show the current stylist in a list of all the stylists. There’s no way it makes sense to have a Calendar instance with just one single Stylist inside it. This class name needs to change.

Perhaps CalendarStylistListItem is a better name.

class CalendarStylistListItem def initialize(stylist) @stylist = stylist end def manual_position @stylist.new_record? ? salon.highest_manual_order_index + 1 : @stylist.manual_order_index end def alphabetical_position stylist_names = salon.ordered_stylist_names stylist_names << @stylist.name unless stylist_names.member? @stylist.name stylist_names.map(&:downcase).sort!.index(@stylist.name.downcase) end def order_index return -1 unless @stylist.active salon.has_manual_order? ? @stylist.manual_position : @stylist.alphabetical_position end private def salon @stylist.salon end end

Now I can take a closer look at the contents of this class itself. I’m going to focus on this method first:

def manual_position @stylist.new_record? ? salon.highest_manual_order_index + 1 : @stylist.manual_order_index end

Why is salon concerned with highest_manual_order_index ? That seems like too fine-grained a detail for it to care about. That also seems like perhaps a responsibility of the CalendarStylistListItem class. Let’s bring it in.

class CalendarStylistListItem def initialize(stylist) @stylist = stylist end def manual_position @stylist.new_record? ? highest_manual_order_index + 1 : @stylist.manual_order_index end def alphabetical_position stylist_names = salon.ordered_stylist_names stylist_names << @stylist.name unless stylist_names.member? @stylist.name stylist_names.map(&:downcase).sort!.index(@stylist.name.downcase) end def order_index return -1 unless @stylist.active salon.has_manual_order? ? @stylist.manual_position : @stylist.alphabetical_position end private def highest_manual_order_index salon.stylists.map(&:manual_order_index).max end def salon @stylist.salon end end

Now I'll focus on the next method down, alphabetical_position .

def alphabetical_position stylist_names = salon.ordered_stylist_names stylist_names << @stylist.name unless stylist_names.member? @stylist.name stylist_names.map(&:downcase).sort!.index(@stylist.name.downcase) end

Some of this code could certainly be refactored to be tidier, but I don't see a lot with respect to object composition that needs to change. One thing that does stick out to me is salon.ordered_stylist_names . This seems like a concern of the CalendarStylistListItem , not of the salon. Let's bring this method in.

class CalendarStylistListItem def initialize(stylist) @stylist = stylist end def manual_position @stylist.new_record? ? highest_manual_order_index + 1 : @stylist.manual_order_index end def alphabetical_position stylist_names = ordered_stylist_names stylist_names << @stylist.name unless stylist_names.member? @stylist.name stylist_names.map(&:downcase).sort!.index(@stylist.name.downcase) end def order_index return -1 unless @stylist.active salon.has_manual_order? ? @stylist.manual_position : @stylist.alphabetical_position end private def highest_manual_order_index salon.stylists.map(&:manual_order_index).max end def ordered_stylist_names salon.stylists.active.non_receptionist .order(salon.attribute_by_which_to_order_stylists) .map(&:name) end def attribute_by_which_to_order_stylists salon.has_manual_order? ? "stylist.manual_order_index" : "LOWER(stylist.name)" end def salon @stylist.salon end end

In the process of doing this I see one more method that's currently in Salon that would be more appropriate in CalendarStylistListItem : the has_manual_order? method. Let's bring that in and rename it to salon_has_manual_order? .

class CalendarStylistListItem def initialize(stylist) @stylist = stylist end def manual_position @stylist.new_record? ? highest_manual_order_index + 1 : @stylist.manual_order_index end def alphabetical_position stylist_names = ordered_stylist_names stylist_names << @stylist.name unless stylist_names.member? @stylist.name stylist_names.map(&:downcase).sort!.index(@stylist.name.downcase) end def order_index return -1 unless @stylist.active salon_has_manual_order? ? @stylist.manual_position : @stylist.alphabetical_position end private def highest_manual_order_index salon.stylists.map(&:manual_order_index).max end def ordered_stylist_names salon.stylists.active.non_receptionist .order(salon.attribute_by_which_to_order_stylists) .map(&:name) end def attribute_by_which_to_order_stylists salon_has_manual_order? ? "stylist.manual_order_index" : "LOWER(stylist.name)" end def salon_has_manual_order? salon.stylists.sum(:manual_order_index) > 0 end def salon @stylist.salon end end

Lastly, I'm pretty sure the only method needed from the outside is order_index . Let's make manual_position and alphabetical_position private.

class CalendarStylistListItem def initialize(stylist) @stylist = stylist end def order_index return -1 unless @stylist.active salon_has_manual_order? ? @stylist.manual_position : @stylist.alphabetical_position end private def manual_position @stylist.new_record? ? highest_manual_order_index + 1 : @stylist.manual_order_index end def alphabetical_position stylist_names = ordered_stylist_names stylist_names << @stylist.name unless stylist_names.member? @stylist.name stylist_names.map(&:downcase).sort!.index(@stylist.name.downcase) end def highest_manual_order_index salon.stylists.map(&:manual_order_index).max end def ordered_stylist_names salon.stylists.active.non_receptionist .order(salon.attribute_by_which_to_order_stylists) .map(&:name) end def attribute_by_which_to_order_stylists salon_has_manual_order? ? "stylist.manual_order_index" : "LOWER(stylist.name)" end def salon_has_manual_order? salon.stylists.sum(:manual_order_index) > 0 end def salon @stylist.salon end end

Making use of the new CalendarStylistListItem class

This tidy new PORO is of course only valuable if we can actually use it. How do we do so?

All the logic inside of CalendarStylistListItem seems to exist for the purpose of figuring out the order_index . Externally, it seems that order_index is the only method on the class that gets called.

And it looks like the one place that order_index gets called is in Stylist#make_sure_self_gets_correct_order_index . Currently, that method looks like this:

class Stylist def make_sure_self_gets_correct_order_index self.cached_order_index = order_index self.manual_order_index = manual_position if salon.has_manual_order? end end

On the line self.cached_order_index = order_index , it's of course the Stylist class's own implementation of order_index that's getting called. It was of course our whole aim in this refactoring exercise to be able to remove methods like order_index from the Stylist class (because, again, the stylist's order index on the calendar is a detail that's peripheral to the essence of the Stylist class). So, let's stop calling Stylist#order_index and start calling CalendarStylistListItem#order_index .

This can be achieved in the following way:

class Stylist def make_sure_self_gets_correct_order_index self.cached_order_index = CalendarStylistListItem.new(self).order_index self.manual_order_index = manual_position if salon.has_manual_order? end end

With that change, the tie is severed, and we can now safely delete the order_index from the Stylist class as well as the following methods that now live inside CalendarStylistListItem :

Stylist#manual_position

Stylist#alphabetical_position

Salon#highest_manual_order_index

Salon#has_manual_order?

Salon#attribute_by_which_to_order_stylists

Salon#ordered_stylist_names

That's a big improvement! Previously, we had a lot of logic scattered across two classes, adding distractions and possible confusions to both classes. Now all that logic is kept in one single cohesive class.

Conclusion

The process of creating my PORO, CalendarStylistListItem , didn't require importing any libraries or learning any radically new techniques. All I had to do was identify a "missing abstraction" in my Stylist class and then conceive of a new object to bundle it up in.

The overall impact to the Stylist class was not all that profound, but cleaning up a big messy class is of course going to take quite a lot of work. I feel like this was a meaningful step in a positive direction.

Next time you encounter a bloated Active Record model, I hope that instead of reaching for a service object or Interfactor, you might consider refactoring to a PORO.