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_assignedanddate_duefields 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.
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 DRY 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.
