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_positionStylist#alphabetical_positionSalon#highest_manual_order_indexSalon#has_manual_order?Salon#attribute_by_which_to_order_stylistsSalon#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.
