---
title: Refactoring with an Apprentice
teaser: |
  Walk through a refactoring of Ruby on Rails test code
  and observe lessons in Rails idioms,
  setting up only necessary test data,
  and extracting methods.
tags: web,good code,testing,apprenticeship
author: Eric Allen
published_on: 2014-05-19
---

When I was part of [thoughtbot][thoughtbot]'s [apprentice.io][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][anthony] became one
of my favorite exercises.

[thoughtbot]: http://thoughtbot.com
[apprentice.io]: http://www.apprentice.io
[anthony]: https://twitter.com/anthonynavarre

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

```ruby
# 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][repo] to follow along.

[breakable_toy]: http://redsquirrel.com/dave/work/a2j/patterns/BreakableToys.html
[repo]: https://github.com/iericallen/refactoring_with_an_apprentice

## 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:

```ruby
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:

```ruby
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][repo_step_1].

[repo_step_1]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/9c4f3cebf0ddcd2d35498d5414975130f07f04da

## 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.

```ruby
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

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

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

[repo_step_2]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/1b274722e2fb2a07516278e630cb61faefb8f80e

## Extract 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 <abbr title="Don't Repeat
Yourself">DRY</abbr> this up by creating a method that will make these
selections for us.

First, we write the method call:

```ruby
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:

```ruby
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][repo_step_3].

[repo_step_3]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/26ba7b8bab4ec4730bc5c3b4211316cc70e4385a

## 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.

```ruby
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][repo_step_4].

[repo_step_4]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/76547013f5a8a0c1b1933211051bd5e7b3720d21

## 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 <abbr title="Don't Repeat Yourself">DRY</abbr> up this
duplication.

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

```ruby
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.

```ruby
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][repo_step_5].

[repo_step_5]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/cefc14421890c1a6dae211eab66c8745ae68fc0d

## 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:

```ruby
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][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:

```ruby
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][repo_step_6].

[form_for]: http://guides.rubyonrails.org/form_helpers.html
[repo_step_6]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/fd8b5ce7ffcc83d374e441dcf3faf283ffa0d5c5

## 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 `yield`s the prefix. We'll call this
method `within_form(prefix, &block)`:

```ruby
def within_form(prefix, &block)
  yield prefix
end
```

Update the method calls to fill in our form:

```ruby
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 `yeild`ing 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][repo_step_7].

[repo_step_7]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/4bc59297e3b4bf52af1cef02169b7aae8bd6b966

## 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:

```ruby
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.

```ruby
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 `yield`ed. We call our form methods, and pass along the `field` and `value`
arguments that each method requires:

```ruby
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][capybara]
`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:

```ruby
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`:

```ruby
attr_reader :prefix, :context
```

Our test is green, so we [commit][repo_step_8].

[capybara]: https://github.com/thoughtbot/capybara-webkit
[repo_step_8]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/c0635f82ad419340c0e75da5152f1ee1bb497aab

## 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:

```ruby
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:

```ruby
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.

```ruby
alias :select_dates :select_date
```

The test is green, [commit][repo_step_9].

[repo_step_9]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/371ab3acae950fe361117e4b99cf41de1e17bb80

## 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:

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

Implement `#submit` in `FormCompletionHelper`:

```ruby
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][repo_step_10].

[repo_step_10]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/52a60eebede3d70a54f6b0ae97bf08b3c78904b6

## 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.

```ruby
# 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.

```ruby
# 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`.

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

Our test is green, so we [commit][repo_step_11].

[repo_step_11]: https://github.com/iericallen/refactoring_with_an_apprentice/commit/0319a0ebaba341c643a992c513cf434a0e39403d

## We're done

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

[formulaic]: http://github.com/thoughtbot/formulaic

## What's next

If you found this post helpful, check out our refactoring
[screencast][learn_refactor] on [Upcase][upcase].

[upcase]: https://thoughtbot.com/upcase/join
[learn_refactor]: https://thoughtbot.com/upcase/humans-present-refactoring

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

[i18n_blog]: https://thoughtbot.com/blog/better-tests-through-internationalization