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.