Golden Master Testing: Controller Refactoring

Abstract background of a variety of metal parts

In the previous article, we used the golden master technique to dump the entire body of an HTTP response to a file. This file was a quick-and-dirty validation that our changes did not alter the behavior of the system.

With the test in place, we removed logic from the view templates and ultimately deleted a whole template, calling one template from two different controller actions. This refactoring improved the view layer, but in the process it added more logic and more duplication to some already janky controller actions.

In this article, we’re going to turn our attention to the controller.

Taking Stock

We have two controller actions that are 40 lines of code each.

Those 80 lines comprise queries, computation, simple data lookup, calls to helper methods, and 18 instance variable assignments.

Brace yourself, here’s what we’re dealing with:

def actions_visible_running_time_data
  @title = t('stats.current_running_time_of_incomplete_visible_actions')
  @actions_running_time = current_user.todos.not_completed.not_hidden.not_deferred_or_blocked.
    select("todos.created_at").
    reorder("todos.created_at DESC")

  @max_weeks = difference_in_weeks(@today, @actions_running_time.last.created_at)
  @actions_running_per_week_array = convert_to_weeks_from_today_array(@actions_running_time, @max_weeks+1, :created_at)

  # cut off chart at 52 weeks = one year
  @count = [52, @max_weeks].min

  # convert to new array to hold max @cut_off elems + 1 for sum of actions after @cut_off
  @actions_running_time_array = cut_off_array_with_sum(@actions_running_per_week_array, @count)
  @max_actions = @actions_running_time_array.max

  # get percentage done cumulative
  @cumulative_percent_done = convert_to_cumulative_array(@actions_running_time_array, @actions_running_time.count )

  @url_labels = Array.new(@count){ |i| url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => i, :id=> "avrt", :only_path => true) }
  @url_labels[@count]=url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => @count, :id=> "avrt_end", :only_path => true)

  @time_labels         = Array.new(@count){ |i| "#{i}-#{i+1}" }
  @time_labels[0] = "< 1"
  @time_labels[@count] = "> #{@count}"

  @y_legend = t('stats.running_time_legend.actions')
  @y2_legend = t('stats.running_time_legend.percentage')
  @x_legend = t('stats.running_time_legend.weeks')
  @values = @actions_running_time_array.join(",")
  @links = @url_labels.join(",")
  @values_2 = @cumulative_percent_done.join(",")
  @x_labels = @time_labels.join(",")
  # add one to @max for people who have no actions completed yet.
  # OpenFlashChart cannot handle y_max=0
  @y_max = 1+@max_actions+@max_actions/10

  render :actions_running_time_data, :layout => false
end

def actions_running_time_data
  @title = t('stats.running_time_all')
  @actions_running_time = current_user.todos.not_completed.select("created_at").reorder("created_at DESC")

  # convert to array and fill in non-existing weeks with 0
  @max_weeks = difference_in_weeks(@today, @actions_running_time.last.created_at)
  @actions_running_per_week_array = convert_to_weeks_from_today_array(@actions_running_time, @max_weeks+1, :created_at)

  # cut off chart at 52 weeks = one year
  @count = [52, @max_weeks].min

  # convert to new array to hold max @cut_off elems + 1 for sum of actions after @cut_off
  @actions_running_time_array = cut_off_array_with_sum(@actions_running_per_week_array, @count)
  @max_actions = @actions_running_time_array.max

  # get percentage done cumulative
  @cumulative_percent_done = convert_to_cumulative_array(@actions_running_time_array, @actions_running_time.count )

  @url_labels = Array.new(@count){ |i| url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => i, :id=> "art", :only_path => true) }
  @url_labels[@count]=url_for(:controller => 'stats', :action => 'show_selected_actions_from_chart', :index => @count, :id=> "art_end", :only_path => true)

  @time_labels         = Array.new(@count){ |i| "#{i}-#{i+1}" }
  @time_labels[0] = "< 1"
  @time_labels[@count] = "> #{@count}"

  # Normalize all the variable names
  @y_legend = t('stats.running_time_legend.actions')
  @y2_legend = t('stats.running_time_legend.percentage')
  @x_legend = t('stats.running_time_legend.weeks')
  @values = @actions_running_time_array.join(",")
  @links = @url_labels.join(",")
  @values_2 = @cumulative_percent_done.join(",")
  @x_labels = @time_labels.join(",")
  # add one to @max for people who have no actions completed yet.
  # OpenFlashChart cannot handle y_max=0
  @y_max = 1+@max_actions+@max_actions/10

  render :layout => false
end

If your eyes just glazed over and you felt like whimpering, crawling into bed, and reading comic books with a flashlight then, rest assured, the distress you are feeling is perfectly normal.

However, if you steel yourself and compare the two methods line by line, you’ll discover that the two methods are almost exactly alike.

Almost.

There are three differences, total.

The key used to populate @title
The query that stores things into @actions_running_time
The :id in the url_for

What I really want is this:

def something_something_data
  title = t('some.title.key')
  records = Stats::SomeStuff.for(current_user)
  @something = Stats::Something.new(title, records, "a_key")
  render :something, layout: false
end

The mess is still there, hiding within Stats::Something. We’re not going to worry about making that code readable, we’re just going to move it with the safety of the golden master tests that we used before.

Tentative Beginnings

First, we need to make sure that the tests are passing:

ruby -Itest test/functional/lockdown_test.rb

If they’re not, it’s because mumble mumble timezones, and you should just move the received.html file over the approved.html file.

Next, we’re going to rearrange the code a little bit to separate the different from the same in our two methods.

Extract the :id used in url_for into an instance variable, and stick it at the top of the method with the other differences.

If you’re thinking eeew, another instance variable??!? then I applaud your instincts. We’re still going to do it that way, because it will let us move the code more easily.

Run the tests again to make sure that the extract variable refactoring worked.

Now we have 3 lines of differences at the top of the two methods, followed by 37 lines of code that is identical in both places.

Note that the very last line, render, is perfectly legitimate controller-y stuff, and we’re not going to touch it.

In good refactoring form, we’re going to copy, not cut all the identical code (except the render), and stick it in app/models/stats/running_time_data.rb:

module Stats
  class RunningTimeData
    def initialize(title, actions_running_time, key)
      @title = title
      @actions_running_time = actions_running_time
      @key = key
    end

    def compute
      # 36 lines of scary code
    end
  end
end

The controller still has all of its code, the tests should be passing.

Now instantiate Stats::RunningTimeData inside of one of the controller actions, and call compute on it. For now we’re not going to use the results, we’re just going to see if the code can run.

def actions_running_time_data
  @title = t('stats.running_time_all')
  @actions_running_time = current_user.todos.not_completed.select("created_at").reorder("created_at DESC")
  @key = "art"
  @data = Stats::RunningTimeData.new(@title, @actions_running_time, @key)
  @data.compute
  # 37 more lines of code
end

The tests fail with the following message:

NoMethodError: undefined method `difference_in_weeks' for #<Stats::RunningTimeData:0x007fd367920d28>

Chasing Down Dependencies

We’re missing a helper method. We’re going to have to copy it from the controller into our new Stats::RunningTimeData class.

When we do that, we get a new error message.

NoMethodError: undefined method `difference_in_days' for #<Stats::RunningTimeData:0x007fba3dfedec8>

When we bring that one over, the test complains:

NoMethodError: undefined method `utc' for nil:NilClass

Here’s the code that is blowing up:

# assumes date1 > date2
def difference_in_days(date1, date2)
  return ((date1.utc.at_midnight-date2.utc.at_midnight)/SECONDS_PER_DAY).to_i
end

So, either date1 or date2 is nil. Tracing through the code, it turns out that date1 is represented by an instance variable @today which is not defined anywhere in our new class.

The controller has defined this value in a before filter.

Make sure that RunningTimeData accepts the value and stores it in an instance variable named @today, and then pass it along when the object gets instantiated in the controller:

@data = Stats::RunningTimeData.new(@title, @actions_running_time, @key, @today)

That fixes one error message, and gives us a new one. This time, we’re missing the constant Stats::RunningTimeData::SECONDS_PER_DAY.

Copy that in, and follow the trail of NoMethodError messages until you’ve dragged all of the following into the new class from the controller.

convert_to_weeks_from_today_array
convert_to_array
cut_off_array_with_sum
convert_to_cumulative_array

Remember that you need to be copying. If you remove any code from the controller, it will be hours before anything passes again.

Guess how I know that.

The final error is another NoMethodError, but this isn’t a method defined in the controller, it’s url_for, which is one of the Rails url helpers.

That’s OK, we can include them!

module Stats
    class RunningTimeData
      include Rails.application.routes.url_helpers
      # …
    end
  end

This, finally, gets the tests passing.

How is it Looking?

Well, we were going to isolate 40 lines of code into a new class, and now we have 100 lines of code.

Moreover, all of those lines of code still exist in the controller.

Things are not really a whole lot better yet.

That said, this gives us an idea of the size of the problem, and that controller code will go away eventually, so I’m going to call this a win.

A very small, very messy win.

Sidestepping the Controller

The controller has an instance of the new Stats::RunningTimeData object stored in @data, which means that the view has access to it.

In the first line of the view, we use @title:

&title=<%= @title -%>,{font-size:16},&

If we add an attr_reader :title in Stats::RunningTimeData, then we can change the view to read:

&title=<%= @data.title -%>,{font-size:16},&

Of course, now the tests fail because we have two actions using the same view, and one of the actions doesn’t have a @data variable yet.

The easiest thing to do is to just set the other action up with some @data. Copy the two lines down to the other action, and the tests should be passing again.

Now, one at a time, introduce an attr_reader for the value that is needed in the view, and swap the view over to read from the @data object.

Remember to run the tests for every change.

This should be smooth sailing, but it isn’t. On line two, when we swap in @data.y_legend we get a failure with the most peculiar diff:

< &y_legend=&amp;lt;p&gt;stats.running_time_legend.actions&lt;/p&gt;,10,0x736AFF&
---
> &y_legend=Actions,10,0x736AFF&

The response body used to say Actions, but now it’s saying: <p>stats.running_time_legend.actions</p>.

What on earth is going on here?

Stick a call to pry from within Stats::RunningTimeData#compute so that we can ask where the method t is defined.

binding.pry
@y_legend = t('stats.running_time_legend.actions')

At the pry prompt, type t, which will blow up, providing a handy stack trace:

[1] pry(#<Stats::RunningTimeData>)> t
ArgumentError: wrong number of arguments (0 for 1)
from /Users/kytrinyx/.gem/ruby/2.1.0/gems/RedCloth-4.2.9/lib/redcloth/erb_extension.rb:16:in `textilize'

RedCloth! That’s not what I expected. The t() in the model is RedCloth’s textilize(), whereas the t() in the controller I18n’s translate, both of which have the short-hand t().

We can fix this by explicitly calling t on I18n inside of Stats::RunningTimeData#compute:

@y_legend = I18n.t('stats.running_time_legend.actions')
@y2_legend = I18n.t('stats.running_time_legend.percentage')
@x_legend = I18n.t('stats.running_time_legend.weeks')

The tests are passing again, and we can go back to the tedious, one-by-one process of exposing attr_readers on Stats::RunningTimeData.

attr_reader :title, :y_legend, :y2_legend, :x_legend, :values, :links, :values_2, :x_labels, :y_max

There are no further surprises, and we can pare the controller actions down:

def actions_visible_running_time_data
  title = t('stats.current_running_time_of_incomplete_visible_actions')
  actions_running_time = current_user.todos.not_completed.not_hidden.not_deferred_or_blocked.
    select("todos.created_at").
    reorder("todos.created_at DESC")
  @data = Stats::RunningTimeData.new(title, actions_running_time, "avrt", @today)
  @data.compute
  render :actions_running_time_data, :layout => false
end

def actions_running_time_data
  title = t('stats.running_time_all')
  actions_running_time = current_user.todos.not_completed.select("created_at").reorder("created_at DESC")
  @data = Stats::RunningTimeData.new(title, actions_running_time, "art", @today)
  @data.compute
  render :layout => false
end

Conclusion

Is this any better?

The controller and the view are now only sharing a single instance variable. That’s definitely an improvement.

The controller actions themselves are down to 5 lines each, which is better than 40, but still not ideal. The queries are still ugly. We could tuck those away into some other object.

It’s annoying that we have to call compute on data.

And how about Stats::RunningTimeData?

It’s pretty awful, actually. It’s 100 lines long. It doesn’t have any duplication, but it has a monster compute method and some incomprehensible helper methods.

In other words, we’re not quite done yet. Roll up your sleeves, grab a snack, and get ready to refactor that model!