Refactoring with an Apprentice

When I was part of thoughtbot’s apprentice.io program, I spent my days learning to code in a maintainable and scalable way. In my first two weeks, refactoring with my mentor Anthony became one of my favorite exercises.

In my third week as an apprentice, we sat down to refactor a test I wrote for my breakable toy application. This is the process we followed:

# spec/features/teacher/add_an_assignment_spec.rb

require 'spec_helper'

feature 'teacher adding an assignment' do
  scenario 'can create an assignment with valid attributes' do
    teacher = create(:teacher)
    course1 = create(:course, name: 'Math', teacher: teacher)
    course2 = create(:course, name: 'Science', teacher: teacher)
    course3 = create(:course, name: 'History', teacher: teacher)
    course4 = create(:course, name: 'Quantum Physics', teacher: teacher)

    visit new_teacher_assignment_path(as: teacher)

    select 'Science', from: :assignment_course_id
    fill_in :assignment_name, with: 'Pop Quiz'
    fill_in :assignment_description, with: 'I hope you studied!'
    select '2014', from: :assignment_date_assigned_1i
    select 'January', from: :assignment_date_assigned_2i
    select '15', from: :assignment_date_assigned_3i
    select '2014', from: :assignment_date_due_1i
    select 'January', from: :assignment_date_due_2i
    select '17', from: :assignment_date_due_3i
    fill_in :assignment_points_possible, with: 100
    click_button I18n.t('helpers.submit.create', model: 'Assignment')

    expect(current_path).to eq(teacher_assignments_path)
    expect(page).to have_content('Course: Science')
    expect(page).to have_content('Name: Pop Quiz')
    expect(page).to have_content('Description: I hope you studied!')
    expect(page).to have_content('Date assigned: January 15, 2014')
    expect(page).to have_content('Date due: January 17, 2014')
    expect(page).to have_content('Points possible: 100')
  end
end

Check out this repo to follow along.

Key areas to improve on:

  • The section of code for filling out the form is not very legible especially the date fields.
  • The date_assigned and date_due fields are not named using Rails idioms.
  • There is a lot of repetition.
  • The code is not reusable.
  • It is important to abstract as many strings from your code as possible to make future changes less painful.

Where to begin?

Idiomatic date attributes

First we looked at some low hanging fruit. Anthony asked me, “when we use t.timestamps in a migration, what are the attribute names?” I knew immediately where he was going with this. Timestamps are named created_at and updated_at. The reason they are named with _at is because it gives developers context that says, “this is a DateTime.” Similarly, when naming Date attributes, the proper Rails idiom is _on.

This may seem like a small thing, but following conventions makes it much easier for other developers (including my future self) to read and derive context about my attributes without having to dig.

So, we changed date_assigned and date_due to assigned_on and due_on respectively:

select '2014', from: :assignment_assigned_on_1i
select 'January', from: :assignment_assigned_on_2i
select '15', from: :assignment_assigned_on_3i
select '2014', from: :assignment_due_on_1i
select 'January', from: :assignment_due_on_2i
select '17', from: :assignment_due_on_3i

Updated assertions to follow the same convention:

expect(page).to have_content('Assigned on: January 15, 2014')
expect(page).to have_content('Due on: January 17, 2014')

We check to make sure our test passes and make a commit.

Create what you need, and nothing more

This test creates 4 courses, so that a teacher must select which course the assignment belongs to. This could be done much more efficiently by using some simple Ruby. However, upon further consideration, having more than 1 course is not only unnecessary, but detrimental to our test because it writes unnecessary data to the database and increases run time.

teacher = create(:teacher)
course1 = create(:course, name: 'Math', teacher: teacher)
course2 = create(:course, name: 'Science', teacher: teacher)
course3 = create(:course, name: 'History', teacher: teacher)
course4 = create(:course, name: 'Quantum Physics', teacher: teacher)

becomes

teacher = create(:teacher)
create(:course, name: 'Science', teacher: teacher)

Again, we check to make sure the test still passes and commit.

Abstract clunky code to methods to increase readability

Anthony is always pushing me to write code so that it performs like it reads. Doing so ensures readability for myself and other developers, and forces us to make sure our objects and methods execute the intentions indicated by their name.

When Anthony isn’t around for conversations, I find pseudocoding helps organize my intentions:

# I want a method that:
#  fills out all attributes within my assignment form
#  selects a course from a dropdown list
#  fills in text fields with the appropriate attributes
#  selects dates for the appropriate attributes
#  submits my form
#  is readable and intention-revealing

The ugliest part of this test (in my opinion) is the way the date fields are selected. It takes 3 lines of code for each date because the form has a separate select for each Month, Day and Year. We can DRY (Don’t Repeat Yourself) this up by creating a method that will make these selections for us.

First, we write the method call:

select_date(:assignment, :assigned_on, 'January 15, 2014')
select_date(:assignment, :due_on, 'January 17, 2014')

When using form_for, form fields are all prefixed with the object they belong to (in this case, :assignment), so we pass that first. Second, we pass the form field we want to select. Third, we pass the date in a string.

Now we write the method:

def select_date(prefix, field, date)
  parsed_date = Date.parse(date)
  select parsed_date.year, from: :"#{ prefix }_#{ field }_1i"
  select parsed_date.strftime('%B'), from: :"#{ prefix }_#{ field }_2i"
  select parsed_date.day, from: :"#{ prefix }_#{ field }_3i"
end

To make extracting the Month, Day and Year from our string much easier, we convert them to a Date object and interpolate the form attributes using the prefix and field arguments.

The test is green, so we commit.

Extract parsing of Dates

We don’t love the local variable for parsing our date and there is an argument order dependency as well. So we extract the parsing of dates to local variables at the top of the test. We aren’t exactly sure what to do about the argument order dependency at this point, and assume this may be an issue with the next form field as well, so we put off that refactor for now.

assigned_on = Date.parse('January 15, 2014')
due_on = Date.parse('January 17, 2014')
...

select_date(:assignment, :assigned_on, assigned_on)
select_date(:assignment, :due_on, due_on)
...

def select_date(prefix, field, date)
  select date.year, from: :"#{ prefix }_#{ field }_1i"
  select date.strftime('%B'), from: :"#{ prefix }_#{ field }_2i"
  select date.day, from: :"#{ prefix }_#{ field }_3i"
end

We feel good about this refactor. Our code is still readable and introduction of the Date object will allow us to put the date in whatever format we like and still achieve the same result.

The test is green, so we commit.

Find similarities in your code

We have 3 text_field attributes being filled the exact same way. Following what we did with select_date, we can write a method that will fill in the text_field’s and will DRY up this duplication.

Again, we write the method call so that it reads as the code performs:

fill_in_text_field(:assignment, :name, 'Pop Quiz')
fill_in_text_field(:assignment, :description, 'I hope you studied!')
fill_in_text_field(:assignment, :points_possible, 100)

This isn’t much of an improvement. While we may have decreased a little bit of ‘noise’ and increased the legibility, we are accomplishing our task in the same number of lines and it is still repetitive. Stay tuned, we’re going somewhere with this.

def fill_in_text_field(prefix, field, value)
  fill_in "#{ prefix }_#{ field }", with: value
end

We are getting less out of this refactor than the last. We still have argument order dependencies and the code’s readability is much the same. However, notice the pattern forming between all the different form methods…

Our test is green, so we commit.

Be patient, go step-by-step

We have a lot of duplication. We are calling the methods select_date and fill_in_text_field multiple times each. We need to find a way (perhaps an object or method) to pass each form field type multiple arguments and iterate over them.

However, we need to make sure that all the form fields are essentially using the same format before we try to wrap them. We need to create a method for selecting our course. Write the method call:

select_from_dropdown(:assignment, :course_id, 'Science')

This may look a bit funny, selecting a course from a drop-down list with a field identifier of :course_id, but passing it the string 'Science' instead of an id. This is due to a little bit of form_for magic. When courses are created, each receives an id when persisted to the database. In our form, we want to be able to select courses that belong to a specific teacher. form_for allows us to display the names of the courses in the drop-down list, but once selected and submitted, the form actually sends the course_id that is required for our Assignment belongs_to :course association.

The method:

def select_from_dropdown(prefix, field, value)
  select value, from: :"#{ prefix }_#{ field }"
end

Now we can really see a pattern forming. Test is green, commit.

Abstract identical arguments using yield and wrapping method calls

The next refactor is a very small but integral step in making these other refactors worth while. What good are the select_from_dropdown, select_date and fill_in_text_field if we can’t use them every time we need to fill in another form?

In each of these method calls, we are passing a ‘prefix’ (in this case it is :assignment) which will always be present no matter what form you are filling out for a particular object. We can abstract this away by wrapping all of these method calls in another method that simply yields the prefix. We’ll call this method within_form(prefix, &block):

def within_form(prefix, &block)
  yield prefix
end

Update the method calls to fill in our form:

within_form(:assignment) do |prefix|
  select_from_dropdown(prefix, :course_id, 'Science')
  fill_in_text_field(prefix, :name, 'Pop Quiz')
  fill_in_text_field(prefix, :description, 'I hope you studied!')
  select_date(prefix, :assigned_on, assigned_on )
  select_date(prefix, :due_on, due_on )
  fill_in_text_field(prefix, :points_possible, 100)
end

All we are doing here is abstracting the object for the form into an argument, and supplying that argument to each method call with yeild.

But what if instead of yeilding our prefix, we were to yield an object back that understood the methods select_from_dropdown, select_date and fill_in_text_field? If that object understood our prefix, and these methods, we could use within_form for any object we need to fill in a form for.

Our test is green, so we commit.

When applicable, create a new object to handle a single responsibility

This next refactor has a lot of moving pieces. We want to create an object responsible for filling out all form fields. As always, write the method call and describe what you want it to perform:

within_form(:assignment) do |f|
  f.select_from_dropdown(course_id: 'Science')
  f.fill_in_text_field(name: 'Pop Quiz')
  f.fill_in_text_field(description: 'I hope you studied!')
  f.fill_in_text_field(points_possible: 100)
  f.select_date(assigned_on: assigned_on)
  f.select_date(due_on: due_on)
end

In order for within_form to perform properly we must update it to yield an object that understands each method responsible for filling out form fields.

def within_form(form_prefix, &block)
  completion_helper = FormCompletionHelper.new(form_prefix, self)
  yield completion_helper
end

By creating the FormCompletionHelper object, we have a place to put the select_from_dropdown, select_date and fill_in_text_field methods.

Each time within_form is called, a new instance of FormCompletionHelper is yielded. We call our form methods, and pass along the field and value arguments that each method requires:

class FormCompletionHelper
  delegate :select, :fill_in, to: :context

  def initialize(prefix, context)
    @prefix = prefix
    @context = context
  end

  def fill_in_text_field(field, value)
    fill_in :"#{ prefix }_#{ field }", with: value
  end

  def select_date(field, date)
    select date.year, from: :"#{ prefix }_#{ field }_1i"
    select date.strftime('%B'), from: :"#{ prefix }_#{ field }_2i"
    select date.day, from: :"#{ prefix }_#{ field }_3i"
  end

  def select_from_dropdown(field, value)
    select value, from: :"#{ prefix }_#{ field }"
  end

  private

  attr_reader :prefix, :context
end

This was a big refactor. By wrapping the methods select_from_dropdown, select_date and fill_in_text_field within the FormCompletionHelper class, their scope changes and they no longer have access to Capybara’s select and fill_in methods. In order to call these methods from FormCompletionHelper, we delegate to the test context (hence, choosing context for the name of this object). We do this by passing self as an argument to the within_form method and we delegate with the code:

delegate :select, :fill_in, to: :context

We also are able to remove prefix as an argument from each of our methods by defining a getter method for prefix:

attr_reader :prefix, :context

Our test is green, so we commit.

Remove duplication, through iteration

We are calling fill_in_text_field 3 times, select_date twice, and the click_button action that submits our form is reliant on ‘Assignment’ being passed as a string.

Let’s address the duplication:

within_form(:assignment) do |f|
  f.select_from_dropdown(course_id: 'Science')
  f.fill_in_text_fields(
    name: 'Pop Quiz',
    description: 'I hope you studied!',
    points_possible: 100
  )
  f.select_dates(
    assigned_on: assigned_on
    due_on: due_on,
  )
end
click_button I18n.t('helpers.submit.create', model: 'Assignment')

Notice that instead of passing multiple arguments, we now pass key: value pairs.

Now, we must adjust our methods to allow one or more fields:

def fill_in_text_field(options)
  options.each do |field, value|
    fill_in :"#{ prefix }_#{ field }", with: value
  end
end
alias :fill_in_text_fields :fill_in_text_field

def select_date(options)
  options.each do |field, date|
    select date.year, from: :"#{ prefix }_#{ field }_1i"
    select date.strftime('%B'), from: :"#{ prefix}_#{ field }_2i"
    select date.day, from: :"#{ prefix }_#{ field }_3i"
  end
end
alias :select_dates :select_date

def select_from_dropdown(options)
  options.each do |field, value|
    select value, from: :"#{ prefix }_#{ field }"
  end
end
alias :select_from_dropdowns :select_from_dropdown

In addition to adding iteration to each of these methods, we also create an alias for each method to allow singular or plural method calls.

alias :select_dates :select_date

The test is green, commit.

Complete abstractions

We know that in the context of a database backed object, form submit actions are only used to create or update the object. To make this submission dynamic we need to assign the model and stipulate whether the action is a create or update.

The method call as we want to see it perform:

def within_form(:assignments) do |f|
...
  f.submit(:create)
end

Implement #submit in FormCompletionHelper:

delegate :select, :fill_in, :click_button, to: :context
...
def submit(create_or_update)
  raise InvalidArgumentException unless [:create, :update].include?(create_or_update.to_sym)
  click_button I18n.t("helpers.submit.#{ create_or_update }", model: model_name)
end

private

def model_name
  prefix.to_s.capitalize
end

This refactor completes our ability to dynamically fill out forms. By abstracting the submit action to the FormCompletionHelper we are able to assign the model_name using our form_prefix. We also include an InvalidArgumentException to allow :create or :update arguments only.

Our test is green, so we commit.

Encapsulate behavior and keep code clean and organized through namespacing

Now that we’ve fully abstracted form completion, it doesn’t make much sense to leave #within_form or FormCompletionHelper in our spec/features/teacher/adding_an_assignment_spec.rb. We can move them to separate files, wrap them in a module, and include them in our RSpec configuration so that our teacher/adding_an_assignment_spec.rb will have access to them. A nice by-product of doing this is that any other feature specs that require a form to be filled out can now use the within_form method.

We move our FormCompletionHelper class to a new file called form_completion_helper.rb. The functionality that this class offers is only going to be used in feature spec files, so we place the file in the spec/support/features/ directory. We also namespace the class by wrapping it in a Features module.

# spec/support/features/form_completion_helper.rb

module Features
  class FormCompletionHelper
    delegate :select, :fill_in, :click_button, to: :context

    def initialize(prefix, context)
      @prefix = prefix
      @context = context
    end

    def fill_in_text_field(options)
      options.each do |field, value|
        fill_in :"#{ prefix }_#{ field }", with: value
      end
    end
    alias :fill_in_text_fields :fill_in_text_field

    def select_date(options)
      options.each do |field, date|
        select date.year, from: :"#{ prefix }_#{ field }_1i"
        select date.strftime('%B'), from: :"#{ prefix }_#{ field }_2i"
        select date.day, from: :"#{ prefix }_#{ field }_3i"
      end
    end
    alias :select_dates :select_date

    def select_from_dropdown(options)
      options.each do |field, value|
        select value, from: :"#{ prefix }_#{ field }"
      end
    end
    alias :select_from_dropdowns :select_from_dropdown

    def submit(create_or_update)
      raise InvalidArgumentException unless [:create, :update].include?(create_or_update.to_sym)
      click_button I18n.t("helpers.submit.#{create_or_update}", model: model_name)
    end

    private

    def model_name
      prefix.to_s.capitalize
    end

    attr_reader :prefix, :context
  end
end

We also created a form_helpers.rb file for our #within_form helper method. We also namespace this method in the Features and FormHelpers modules.

# spec/support/features/form_helpers.rb

module Features
  module FormHelpers
    def within_form(form_prefix, &block)
      completion_helper = FormCompletionHelper.new(form_prefix, self)
      yield completion_helper
    end
  end
end

The last step is to require these modules in /spec/spec_helper.rb.

RSpec.configure do |config|
  config.include Features
  config.include Features::FormHelpers
end

Our test is green, so we commit.

We’re done.

Next time, I might use the Formulaic gem. However, I find that exercises like this really help me understand the process of writing quality code.

What’s next?

If you found this post helpful, check out our refactoring screencast on Upcase.

Also, check out this blog to take your test refactoring to the next level using I18n.

Hound automatically reviews Ruby, JavaScript, and CoffeeScript code in your GitHub pull requests and comments on style violations. It is free for open source repos and $12/month per private repo.