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.

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.

and 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?

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.

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.

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.

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.

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.

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.

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

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

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 yield ed. 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.

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.

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.

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.

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.

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