I work for a University, though not as a professor. I do have the privilege to work with students in a variety of capacities, including web application development. There are several departments on campus that offer web development courses but they tend to focus on basic tasks performed with PHP rather than a greater understanding of the industry and the options within. Over the years I have been able to introduce many students to using Ruby and Rails as well as the underlying concepts perpetuated by agile development.

One of the key concepts is the idea of refactoring. Core to test driven development and essential to keeping your code DRY¹, this concept is often not even discussed in the introductory development courses or, if it is taught, is overlooked by the students. Simply defined, refactoring is the process of rewriting code to make it easier to maintain while not altering the behavior of the application. When done well you eliminate complexities while making the code easier to work with. As I look at the attributes of a great craftsmen I see two that can be attributed with refactoring; aspire to improve and maintain cleanliness.

The first weeks of employment of a new developer on my staff consists of going through online courses² and reading well-written books.³ Then, we spend some time pair-programming where we share one workstation with one of us driving and the other navigating.⁴ Through this process the new developer often gets their first taste of refactoring code. I have repeated this cycle many times as I train new developers and, the most recent time, I had the idea to document our refactoring for future reference.

Refactoring In Practice

The following is a summary of conversation and actions performed by myself and one of the student developers.

Our active user story⁵ was to provide date ranges for an application period, interview sign up period, and interview session period for an employment application feature of the website. The current implementation was displaying the beginning and ending dates on individual lines. You can also see that the interviews_begin_on and interviews_end_on attributes are optional.

1
2
3
4
5
6
7
8
9
10
11
12
13
14

# list_display is a helper for creating DL elements
# def list_display(label, content)
#   capture_haml do
#     haml_tag :dt, label
#     haml_tag :dd do
#       haml_concat content
#     end
#    end
#  end
%dl
  = list_display 'Apps Accepted Beginning', @ra_app_term.available_at.to_s(:typical)
  = list_display 'Apps No Longer Accepted', @ra_app_term.unavailable_at.to_s(:typical)
  = list_display 'Interviews Begin', @ra_app_term.interviews_begin_on.to_s(:long) if @ra_app_term.interviews_begin_on.present?
  = list_display 'Interviews End', @ra_app_term.interviews_end_on.to_s(:long) if @ra_app_term.interviews_end_on.present?

Our customer⁶ requested we simplify this display, using a format similar to “10/01/12 - 10/09/12”. The first step is to write a our failing test⁷ with the syntax we plan to use in our view. We also focus only on one piece at a time, so we start with available_at and unavailable_at. We plan to create a method called application_period, so we add the following test.

1
2
3
4
5
6

context "displaying date periods" do
  subject { ra_app_terms(:spring_2013) }
  describe "#application_period" do
    its(:application_period) { should == "09/01/12 - 10/14/12" }
  end
end

Predictably, this test fails since we’ve not created the application_period method. So, we switch to the model and add the method.

1
2
3

def application_period
  "#{self.available_at.to_s(:us_date)} - #{self.unavailable_at.to_s(:us_date)}"
end

Our test now passes so we move to the next set of attributes related to interview sign ups. Switching back to our spec file we add another test.

1
2
3

describe "#interview_sign_up_period" do
  its(:interview_sign_up_period) { should == "10/17/12 - 10/23/12" }
end

Skipping forward to making this test pass we add the interview_sign_up_period method to the model.

1
2
3

def interview_sign_up_period
  "#{self.interview_sign_up_at.to_s(:us_date)} - #{self.interview_sign_up_until.to_s(:us_date)}"
end

The test passes and the astute student points out that we could refactor these methods, simplifying the long term maintenance of our code. At this point he becomes the driver and updates the model.

1
2
3
4
5
6
7
8
9
10
11
12

def application_period
  display_date_period(self.available_at, self.unavailable_at)
end

def interview_sign_up_period
  display_date_period(self.interview_sign_up_at, self.interview_sign_up_until)
end

private
def display_date_period(first_date, last_date)
  "#{first_date.to_s(:us_date)} - #{last_date.to_s(:us_date)}"
end

We run our test which returns two passing examples. That’s refactoring in a nutshell. What happened next made this little change extremely important. The phone rang and it was our customer. He expresses that they’d like to add a notice to any applicant letting them know when the interview sign up period is using the format of “October 14 until October 23.” Our refactor meant we could focus our efforts on the new display_date_period method and not the individual date range methods.

I switched to the spec and add the following test and then hand the keyboard over to the student.

1
2
3
4
5
6

describe "#interview_sign_up_period" do
  its(:interview_sign_up_period) { should == "10/17/12 - 10/23/12" }
  it "allows optional date_format" do
    subject.interview_sign_up_period(date_format: :month_day).should == "October 17 - October 23"
  end
end

My student developer drives the following change to our model to handle the now optional date format.

1
2
3
4
5
6
7
8
9
10
11
12

def application_period(options={})
  display_date_period(self.available_at, self.unavailable_at, options)
end

def interview_sign_up_period(options={})
  display_date_period(self.interview_sign_up_at, self.interview_sign_up_until, options)
end

private
def display_date_period(first_date,last_date,options)
  "#{first_date.to_s(options[:date_format])} - #{last_date.to_s(options[:date_format])"
end

The model changes just made are a prime example as to why we write tests. After I kept quiet and let the tests run my student was surprised to see our previously successful tests were failing, while the new test passed. I pointed out to him that the output from the display_date_period method is expecting options[:date_format] to contain a value.

At this point I take the liberty to point out a couple of other Rubyisms we could use to better equip ourselves for future development. I explain the splat operator⁸, extract_options!, and join. These will each help us handle the latest request from the customer.

I take over as driver and update display_date_period method to handle an options hash.

1
2
3
4
5
6
7

private
def display_date_period(*dates)
  options = dates.extract_options!
  options[:date_format] ||= :us_date
  options[:separator] ||= ' - '
  dates.collect { |date| date.to_s(options[:date_format]) }.join(options[:separator])
end

We re-run the test suite and we have three passing tests. My student drives again and writes the next test to handle passing optional joining text.

1
2
3
4
5
6
7
8
9

describe "#interview_sign_up_period" do
  its(:interview_sign_up_period) { should == "10/17/12 - 10/23/12" }
  it "allows optional date_format" do
    subject.interview_sign_up_period(date_format: :month_day).should == "October 17 - October 23"
  end
  it "allows optional separator" do
    subject.interview_sign_up_period(separator: ' until ').should == "10/17/12 until 10/23/12"
  end
end

The previous update to display_date_period set the method up to handle an optional separator being defined, so our new tests run without any issue. My student, interested in seeing whether my code would actually handle multiple options adds one additional test.

1
2
3
4
5
6
7
8
9
10
11
12

describe "#interview_sign_up_period" do
  its(:interview_sign_up_period) { should == "10/17/12 - 10/23/12" }
  it "allows optional date_format" do
    subject.interview_sign_up_period(date_format: :month_day).should == "October 17 - October 23"
  end
  it "allows optional separator" do
    subject.interview_sign_up_period(separator: ' until ').should == "10/17/12 until 10/23/12"
  end
  it "allows multiple options" do
    subject.interview_sign_up_period(date_format: :month_day, separator: ' until ').should == "October 17 until October 23"
  end
end

The test results all come back green, 5 passing tests. I mention that we haven’t written the test to handle the interview session period. Our initial story requested this feature for application period, interview sign up period, and interview session period. Before I finish talking the student has already written the next test.

1
2
3

describe "#interview_period" do
  its(:interview_period) { should == "10/25/12" }
end

We both notice a problem. According to the original design it is optional to have an end date for the interview period. Our test fixture had a value for interviews_begin_on but had a null value for interviews_end_on. We know the first time running the test will fail because we haven’t defined interview_period, so we switch to the model to add that method. We also decide to handle null values within display_date_period, so that this model looks similar to the other specific date period methods.

1
2
3

def interview_period(options={})
  display_date_period(self.interviews_begin_on, self.interviews_end_on, options)
end

Now we have to determine the best way to handle the possible null date attribute. I suggest we run our test to see what errors we get. The error is wrong number of arguments(1 for 0) in #to_s. I point out that if you call to_s on a null value the result is an empty string. However, unlike the to_s method of a Date or Time object, you cannot provide an argument.

To correct this we can explicitly test that the provided value is a Date or Time object or we can use an inline rescue to handle the exception. I take over driving and use the inline rescue to demonstrate another Rubyism.

1
2
3
4
5
6
7

private
def display_date_period(*dates)
  options = dates.extract_options!
  options[:date_format] ||= :us_date
  options[:separator] ||= ' - '
  dates.collect { |date| date.to_s(options[:date_format]) rescue nil }.join(options[:separator])
end

We run the test suite and our test it fails on our expectation and not because of an exception. The returned result was "10/25/12 - " which means our null value is being accounted for in the join method. We open Ruby-doc.org and look into Array methods to strip out a null element and find two that would work: reject and delete_if. Because we do not need the behavior to be destructive we go with reject and use the proc shortcut⁹ with the blank? method to check for null values.

1
2
3
4
5
6
7

private
def display_date_period(*dates)
  options = dates.extract_options!
  options[:date_format] ||= :us_date
  options[:separator] ||= ' - '
  dates.reject(&:blank?).collect { |date| date.to_s(options[:date_format]) rescue nil }.join(options[:separator])
end

All the tests are now passing. The student, ready to continue with adding edge cases asks “What happens if they pass more than two dates?” and reaches for the keyboard to start driving our next test. I quickly put the proverbial brakes on. We could spend the rest of the day working to accept every possible combination of arguments, but as it stands now we have refactored redundant code and have met the customer’s request. We instead switch gears and update the view to use the new methods.

1
2
3
4

%dl
  = list_display 'Application Period', @ra_app_term.application_period
  = list_display 'Interview Sign-up Period', @ra_app_term.interview_sign_up_period
  = list_display 'Interview Period', @ra_app_term.interview_period

We pull open the page in our browser and everything works as expected. The final step is to add the notice to applicants that they can sign up for an interview.

The previous example was relatively simple, but it is a scenario I see many times while teaching new developers. We refactored early in the process and because we did when the code was still simple, adding more complexity was relatively simple. Ultimately, this could have been handled by a presenter class to keep presentation out of the model and that is exactly what we did in a future update.

The Model

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25

class RaAppTerm < ActiveRecord::Base
  # ... additional code

  def application_period(options={})
    date_period_display self.available_at, self.unavailable_at, options
  end

  def interview_sign_up_period(options={})
    date_period_display self.interview_sign_up_at, self.interview_sign_up_until, options
  end

  def interview_period(options={})
    date_period_display self.interviews_begin_on, self.interviews_end_on, options
  end

  private
  def date_period_display(*dates)
    options = dates.extract_options!
    options[:separator] ||= ' - '
    options[:date_format] ||= :us_date
    dates.reject(&:blank?).collect { |date| date.to_s(options[:date_format]) rescue nil }.join(options[:separator])
  end

  # ... additional code
end

The Spec

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28

require 'spec_helper'

describe RaAppTerm do
  fixtures :ra_app_terms
  # ... additional code
  context "displaying date periods" do
    subject { ra_app_terms(:spring_2013) }
    describe "#application_period" do
      its(:application_period) { should == "09/01/12 - 10/14/12" }
    end
    describe "#interview_sign_up_period" do
      its(:interview_sign_up_period) { should == "10/17/12 - 10/23/12" }
      it "allows optional date_format" do
        subject.interview_sign_up_period(date_format: :month_day).should == "October 17 - October 23"
      end
      it "allows optional separator" do
        subject.interview_sign_up_period(separator: ' until ').should == "10/17/12 until 10/23/12"
      end
      it "allows multiple options" do
        subject.interview_sign_up_period(date_format: :month_day, separator: ' until ').should == "October 17 until October 23"
      end
    end
    describe "#interview_period" do
      its(:interview_period) { should == "10/25/12" }
    end
  end
  # ... additional code
end