Refactoring to POROs (using tests)

by Jason Swett,

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.

  1. Try to find a “missing abstraction” in the method whose bulk you want to reduce, and create a new PORO expressing that abstraction
  2. Identify a line (or group of lines) from the original function you want to move into its own class
  3. Write a failing test for that functionality
  4. Cut and paste the original line into the new PORO to make the test pass (making small adjustments if necessary)
  5. Repeat from step 2 until the PORO code gets too big and messy
  6. Refactor the PORO code until you’re satisfied with its cleanliness
  7. Repeat from step 2
  8. Rejoice

2 thoughts on “Refactoring to POROs (using tests)

  1. Calin

    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.

    Reply

Leave a Reply

Your email address will not be published. Required fields are marked *