For a good chunk of my Rails career I was under the impression that in Rails projects, we have one model per database table. If all I have in my application are three tables, `users`, `products`, and `orders`, then I’ll have exactly three corresponding ActiveRecord classes, `User`, `Product`, and `Order`.
Then I read a post by Steve Klabnik (which I can’t find now) pointing out that the classes in `app/models` don’t have to inherit from ActiveRecord. It was a real aha moment for me and a turning point in the way I write code.
These ActiveRecord-less classes are often called Plain old Ruby Objects or POROs.
Why POROs are useful
Small methods are easier to understand than large methods. Small classes are easier to understand than large classes. Code that’s easy to understand tends to me less expensive to maintain and more enjoyable to maintain.
Rails projects that only use the classes that inherit from ActiveRecord tend to eventually end up with ActiveRecord classes that are way too big and do way too much unrelated stuff. If a project contains a `Product` class, the `Product` class becomes sort of a dumping ground for anything that could be tangentially related to a product. Inside of the huge `Product` class there might be three or four abstractions “hiding” in the code, yearning to be separated into their own neat and tidy classes where they don’t have to live scrambled together with unrelated code.
I’ve also found that when I have a method that’s bigger than I’d like but just seems irreducible, I can pull out that method’s code into a new PORO and suddenly I see how my ugly method can be morphed into a neat and tidy class.
All this is rather abstract so let’s take a look at a concrete example.
An example of refactoring to a PORO
Here’s a piece of code written by a relatively unskilled programmer—me from eight years ago.
Don’t try to understand this method right now. First just observe its size and shape.
class Appointment < ActiveRecord::Base
def self.save_from_params(params)
a = params[:id] ? self.find(params[:id]) : self.new
a.attributes = {
time_block_type: TimeBlockType.find_by_code(params[:appointment][:time_block_type_code]),
notes: params[:appointment][:notes],
stylist_id: params[:appointment][:stylist_id],
is_cancelled: params[:appointment][:is_cancelled],
tip: params[:appointment][:tip],
}
if a.time_block_type_code == "APPOINTMENT"
a.client = self.build_client(params, a.stylist.salon)
else
a.client = Client.no_client
a.length = Appointment.determine_length(
start_time: params[:appointment][:start_time_time],
end_time: params[:appointment][:end_time],
)
end
a.set_time(params[:appointment][:start_time_time], params[:appointment][:start_time_ymd])
a.set_repeat_logic(params)
a.save
a.set_payments_from_json_string(params[:serialized_payments])
a.set_services_and_products_from_json_string(params[:serialized_products_and_services])
a.record_transactions
if !a.new_record?
a.reload.generate_recurrence_hash_if_needed
a.reload.save_future
end
a
end
end
At the time I wrote this method I knew that it was much longer than I’d like but I could figure out how to make it smaller. Now that I’m older and wiser I know that it can be refactored into a new class. I’m going to make use of the factory pattern and call this new class `AppointmentFactory`.
I need to be very careful when creating this new class to ensure that it works precisely the same as the existing method. I’m going to use tests to help reduce the likelihood that I screw anything up. I’m also going to go in very small steps.
Writing the first line of my PORO
The first small step I’m going to take is to move the first line of `save_from_params` into a new `AppointmentFactory` class.
class AppointmentFactory
def self.create(params)
params[:id] ? Appointment.find(params[:id]) : Appointment.new
end
end
The original method will be changed, very slightly, to this:
class Appointment < ActiveRecord::Base
def self.save_from_params(params)
a = AppointmentFactory.create(params)
a.attributes = {
time_block_type: TimeBlockType.find_by_code(params[:appointment][:time_block_type_code]),
notes: params[:appointment][:notes],
stylist_id: params[:appointment][:stylist_id],
is_cancelled: params[:appointment][:is_cancelled],
tip: params[:appointment][:tip],
}
if a.time_block_type_code == "APPOINTMENT"
a.client = self.build_client(params, a.stylist.salon)
else
a.client = Client.no_client
a.length = Appointment.determine_length(
start_time: params[:appointment][:start_time_time],
end_time: params[:appointment][:end_time],
)
end
a.set_time(params[:appointment][:start_time_time], params[:appointment][:start_time_ymd])
a.set_repeat_logic(params)
a.save
a.set_payments_from_json_string(params[:serialized_payments])
a.set_services_and_products_from_json_string(params[:serialized_products_and_services])
a.record_transactions
if !a.new_record?
a.reload.generate_recurrence_hash_if_needed
a.reload.save_future
end
a
end
end
Writing tests for my PORO
Now I’m going to write the first tests for my new `AppointmentFactory` class. I can see based on the code that I’ll have to test two scenarios: 1) when an id is present and 2) when an id is not present.
I’ll start with just the shell of the test.
require 'spec_helper' # Why not request 'rails_helper'? Because this is an old project
RSpec.describe AppointmentFactory do
describe '#create' do
context 'when id is present' do
it 'returns the appointment with that id' do
end
end
context 'when id is not present' do
it 'returns a new appointment instance' do
end
end
end
end
Why do I write just the shell of the test first? The reason is to reduce the amount of stuff I have to juggle in my head. If I get the test cases I want to write out of my head and onto the screen, I’m free to stop thinking about that part of my task.
The first test case I’ll fill in is the scenario where I’m passing the id of an existing appointment.
Test for existing appointment
require 'spec_helper'
RSpec.describe AppointmentFactory do
describe '#create' do
context 'when id is present' do
let(:existing_appointment) { create(:appointment) }
let(:appointment) { AppointmentFactory.create(id: existing_appointment.id) }
it 'returns the appointment with that id' do
expect(appointment.id).to eq(existing_appointment.id)
end
end
context 'when id is not present' do
it 'returns a new appointment instance' do
end
end
end
end
This test passes as expected. I’m skeptical, though. I never trust a test that I only see pass and never see fail. How can I be sure that the test is testing what I think it’s testing? How can I be sure I didn’t just make a mistake in the way I wrote the test?
In order to see the test fail, I’ll comment out the correct line and add something that won’t satisfy the test.
class AppointmentFactory
def self.create(params)
#params[:id] ? Appointment.find(params[:id]) : Appointment.new
Appointment.new
end
end
As I expect, the test now fails.
Failures:
1) AppointmentFactory#create when id is present returns the appointment with that id
Failure/Error: expect(appointment.id).to eq(existing_appointment.id)
expected: 1
got: nil
(compared using ==)
# ./spec/models/appointment_factory_spec.rb:10:in `block (4 levels) in <top (required)>'
Now that I’m comfortable with that test case I’ll move onto the case where I’m not passing the id of an existing appointment.
Test for new appointment instance
require 'spec_helper'
RSpec.describe AppointmentFactory do
describe '#create' do
context 'when id is present' do
let(:existing_appointment) { create(:appointment) }
let(:appointment) { AppointmentFactory.create(id: existing_appointment.id) }
it 'returns the appointment with that id' do
expect(appointment.id).to eq(existing_appointment.id)
end
end
context 'when id is not present' do
let(:appointment) { AppointmentFactory.create }
it 'returns a new appointment instance' do
expect(appointment.id).to be_nil
end
end
end
end
This actually doesn’t pass.
Failures:
1) AppointmentFactory#create when id is not present returns a new appointment instance
Failure/Error: let(:appointment) { AppointmentFactory.create }
ArgumentError:
wrong number of arguments (0 for 1)
# ./app/models/appointment_factory.rb:2:in `create'
# ./spec/models/appointment_factory_spec.rb:15:in `block (4 levels) in <top (required)>'
# ./spec/models/appointment_factory_spec.rb:18:in `block (4 levels) in <top (required)>'
My create
method is expecting an argument but I’m not passing it one. Rather than change my code to pass an empty hash, I’ll change the signature of the create
method to default to an empty hash if no argument is provided.
class AppointmentFactory
def self.create(params = {})
params[:id] ? Appointment.find(params[:id]) : Appointment.new
end
end
Fixing a misnomer
My test now passes. And now that I step back and assess what I’ve done, I realize that create
is probably not the most sensical name for this method. Maybe something like find_or_create
would be more fitting.
class AppointmentFactory
def self.find_or_create(attributes = {})
attributes[:id] ? Appointment.find(attributes[:id]) : Appointment.new
end
end
I’ll of course have to change the naming in the test file as well.
require 'spec_helper'
RSpec.describe AppointmentFactory do
describe '#find_or_create' do
context 'when id is present' do
let(:existing_appointment) { create(:appointment) }
let(:appointment) { AppointmentFactory.find_or_create(id: existing_appointment.id) }
it 'returns the appointment with that id' do
expect(appointment.id).to eq(existing_appointment.id)
end
end
context 'when id is not present' do
let(:appointment) { AppointmentFactory.find_or_create }
it 'returns a new appointment instance' do
expect(appointment.id).to be_nil
end
end
end
end
The very slightly refactored version of my original method
As a reminder, my original save_from_params
method looks like this:
class Appointment < ActiveRecord::Base
def self.save_from_params(params)
a = AppointmentFactory.find_or_create(params)
a.attributes = {
time_block_type: TimeBlockType.find_by_code(params[:appointment][:time_block_type_code]),
notes: params[:appointment][:notes],
stylist_id: params[:appointment][:stylist_id],
is_cancelled: params[:appointment][:is_cancelled],
tip: params[:appointment][:tip],
}
if a.time_block_type_code == "APPOINTMENT"
a.client = self.build_client(params, a.stylist.salon)
else
a.client = Client.no_client
a.length = Appointment.determine_length(
start_time: params[:appointment][:start_time_time],
end_time: params[:appointment][:end_time],
)
end
a.set_time(params[:appointment][:start_time_time], params[:appointment][:start_time_ymd])
a.set_repeat_logic(params)
a.save
a.set_payments_from_json_string(params[:serialized_payments])
a.set_services_and_products_from_json_string(params[:serialized_products_and_services])
a.record_transactions
if !a.new_record?
a.reload.generate_recurrence_hash_if_needed
a.reload.save_future
end
a
end
end
Tackling the second chunk of code, attribute setting
I want to focus now on moving the chunk starting with a.attributes =
out of save_from_params
and into AppointmentFactory
. I’ll start with a test for just the first of those attributes, time_block_type
.
describe 'attributes' do
let!(:time_block_type) { create(:time_block_type, code: 'APPOINTMENT') }
let(:appointment) do
AppointmentFactory.find_or_create(
appointment: { time_block_type_code: 'APPOINTMENT' }
)
end
it 'sets a time block type' do
expect(appointment.time_block_type).to eq(time_block_type)
end
end
I run the test and watch it fail, then add the code to make the test pass.
class AppointmentFactory
def self.find_or_create(attributes = {})
appointment = attributes[:id] ? Appointment.find(attributes[:id]) : Appointment.new
appointment.attributes = {
time_block_type: TimeBlockType.find_by_code(attributes[:appointment][:time_block_type_code])
}
appointment
end
end
Unfortunately it seems that one of my “old” tests is now failing.
Failures:
1) AppointmentFactory#find_or_create when id is present returns the appointment with that id
Failure/Error: let(:appointment) { AppointmentFactory.find_or_create(id: existing_appointment.id) }
NoMethodError:
undefined method `[]' for nil:NilClass
# ./app/models/appointment_factory.rb:6:in `find_or_create'
# ./spec/models/appointment_factory_spec.rb:7:in `block (4 levels) in <top (required)>'
# ./spec/models/appointment_factory_spec.rb:10:in `block (4 levels) in <top (required)>'
Why is this test failing? The problem (which I discovered after much head-scratching) is that if I only pass in something for attributes[:id]
and nothing for attributes[:appointment]
, then the line that tries to access attributes[:appointment][:time_block_type_code]
says “You’re trying invoke the method []
on attributes[:appointment]
, but attributes[:appointment]
is nil
, and NilClass
doesn’t have a method called []
.”
Fair enough. The simplest thing I can think of to get the test passing is just to add a guard clause that returns unless attributes[:appointment]
actually has something in it.
class AppointmentFactory
def self.find_or_create(attributes = {})
appointment = attributes[:id] ? Appointment.find(attributes[:id]) : Appointment.new
return appointment unless attributes[:appointment].present?
appointment.attributes = {
time_block_type: TimeBlockType.find_by_code(attributes[:appointment][:time_block_type_code])
}
appointment
end
end
Testing the rest of the attributes
With that out of the way I’m ready to turn my attention to the rest of the attributes. I’m not feeling a particular appetite to write an individual test for each attribute, and I don’t feel like separate tests would be much more valuable or clear than one single test case with several assertions. (In fact, I think one single test case with several assertions would actually be more clear than if they were separate.)
describe 'attributes' do
let!(:time_block_type) { create(:time_block_type, code: 'APPOINTMENT') }
let!(:stylist) { create(:stylist) }
let(:appointment) do
AppointmentFactory.find_or_create(
appointment: {
time_block_type_code: 'APPOINTMENT',
notes: 'always late',
stylist_id: stylist.id,
is_cancelled: 'true',
tip: '100'
}
)
end
it 'sets the proper attributes' do
expect(appointment.time_block_type).to eq(time_block_type)
expect(appointment.notes).to eq('always late')
expect(appointment.stylist).to eq(stylist)
expect(appointment.is_cancelled).to be true
expect(appointment.tip).to eq(100)
end
end
After running that test and watching it fail I’m ready to pull in the code that sets these attributes.
class AppointmentFactory
def self.find_or_create(attributes = {})
appointment = attributes[:id] ? Appointment.find(attributes[:id]) : Appointment.new
return appointment unless attributes[:appointment].present?
appointment.attributes = {
time_block_type: TimeBlockType.find_by_code(attributes[:appointment][:time_block_type_code]),
notes: attributes[:appointment][:notes],
stylist_id: attributes[:appointment][:stylist_id],
is_cancelled: attributes[:appointment][:is_cancelled],
tip: attributes[:appointment][:tip]
}
appointment
end
end
Now my test passes.
Refactoring my crappy AppointmentFactory code
So far I’ve just been focusing on getting my tests to pass as quickly and easily as possible with little to no regard to code quality. (I picked up this habit from Kent Beck’s Test Driven Development: By Example.)
Now that I have a non-trivial amount of code in my class, I feel ready to do some refactoring for understandability. I can feel confident that correct functionality will be preserved throughout my refactoring thanks to the safety net of my little test suite.
class AppointmentFactory
def self.find_or_create(attributes = {})
Appointment.find_or_initialize_by(id: attributes[:id]).tap do |appointment|
appointment.attributes = filtered_attributes(attributes[:appointment])
end
end
def self.filtered_attributes(appointment_attributes)
return {} unless appointment_attributes.present?
{
time_block_type: TimeBlockType.find_by_code(appointment_attributes[:time_block_type_code]),
notes: appointment_attributes[:notes],
stylist_id: appointment_attributes[:stylist_id],
is_cancelled: appointment_attributes[:is_cancelled],
tip: appointment_attributes[:tip]
}
end
end
The slightly more refactored version of my original method
This is what the save_from_params
method now looks like. Having shaved off 6 lines by moving some code into a PORO, this method is now 24 lines instead of 32. The job is of course far from done but it’s a good start.
class Appointment < ActiveRecord::Base
def self.save_from_params(params)
a = AppointmentFactory.find_or_create(params)
if a.time_block_type_code == "APPOINTMENT"
a.client = self.build_client(params, a.stylist.salon)
else
a.client = Client.no_client
a.length = Appointment.determine_length(
start_time: params[:appointment][:start_time_time],
end_time: params[:appointment][:end_time],
)
end
a.set_time(params[:appointment][:start_time_time], params[:appointment][:start_time_ymd])
a.set_repeat_logic(params)
a.save
a.set_payments_from_json_string(params[:serialized_payments])
a.set_services_and_products_from_json_string(params[:serialized_products_and_services])
a.record_transactions
if !a.new_record?
a.reload.generate_recurrence_hash_if_needed
a.reload.save_future
end
a
end
end
I could of course continue the illustration until the save_from_params
method is down to a one-liner but I think you can see where I’m going, and hopefully this much is enough to convey my methodology.
My “refactor to PORO” methodology, boiled down
Here are the rough steps I’ve used in this post to improve the understandability of my code and put it under test coverage. I use this methodology all the time in production projects, whether it be a greenfield project or legacy project.
- Try to find a “missing abstraction” in the method whose bulk you want to reduce, and create a new PORO expressing that abstraction
- Identify a line (or group of lines) from the original function you want to move into its own class
- Write a failing test for that functionality
- Cut and paste the original line into the new PORO to make the test pass (making small adjustments if necessary)
- Repeat from step 2 until the PORO code gets too big and messy
- Refactor the PORO code until you’re satisfied with its cleanliness
- Repeat from step 2
- Rejoice
Cool article
I will argue that you are using the builder pattern since you are building the same Appoiment object not a family of Appoiment objects.
Interesting. I had not heard of the builder pattern, will check it out.