Live Refactoring

April 28, 2019

[ruby] [refactoring]

I "live" tweeted a refactoring session.

https://twitter.com/kennethmayer/status/1123375649375248385

We have a class that creates custom document names based on a bunch of other inputs. The original class is called FileNaming. It’s worked for almost three years, but time and features have made it more troublesome. Customers want more control, support are carrying a heavier load, everyone wants to know how this thing actually works.

FileNaming uses Ruby’s String/Kernel.sprintf “template” system:

irb: Kernel.format("%{foo} - %{bar}", { foo: "apple", bar: "banana" })
===> "apple - banana"

That’s pretty sweet; it’s very light weight. If you have a typo in the template name, you get a KeyError exception (good for validation before committing the template change to the production database). On the other hand, it’s not really safe for user input, since you’re handing a user-input String to Kernel.format Which was fine. This template system is changed very rarely and only by “highly skilled support personnel” (i.e. me, the rest of engineering and a couple others in the company). It was never modified by or shown to end user/customers. Nowadays, we have a bigger customer base, who want more and more customizations (0-padding to 5 digits instead of 6, using “.” instead of “-“ between date components, and on and on and on).

It is time to move on. I’ve used Liquid, a gem from Shopify. I’m going to use it again. So, starting with the pretty comprehensive test suite for FileNaming, we’ll build a new class, FileNameTemplate that implements all of the old patterns, but using Liquid, plus filters for formatting, instead of the 3x elements that rendered the same thing, but with different padding. Are you with me so far? I have two classes that have near equivalent functionality. I want to gradually move our customers from one templating system to the other. I also want to minimize impacts on our existing code base; to minimize things going wrong.

Here’s a pretty simple and typical example of how the current template system works…

def filename
  template = user.try(:document_filename_template) || FileNaming::DOCUMENT_TEMPLATE
  FileNaming.new(object, template).document_filename(".pdf")
end

I realize that I’m rubber ducking this to Twitter. Please be gentle. I don’t have a SoundCloud account. But support @alsoalsoalso_: They have a new album coming out soonish. Users (and other classes in our application) have a mix-in module, Preferences, that store and manage preference settings with reasonable fall backs. Ideally, all of the uses of FileNaming would not have to change. That would break the smallest number of things. (We have good test coverage so even if I do make a “big” change, I’m confident that we’ll catch regressions before they leave my git index.)

First thoughts The class names are close, but are not symmetric. What am I trying to communicate here? Second thoughts: OODObject Oriented Design eschews if/then/else branches in favor of classes and methods. Send a message to a class and let the class handle the crazy complexities. Third thoughts: It’s Sunday afternoon and the spouse is out of town. I gotta feed the kids soon. Tomorrow is a school day. I can’t code all night. What can I get done quickly?

I’m going to use some OO patterns to switch between the different rendering classes. Ruby doesn’t have abstract classes, per se, so the structure is a bit more ad hoc, but I think you’ll get the idea. And, to quote @KentBeck

Make the change easy to make, then make the easy change.

I’ll start by creating a factory that just recapitulates the existing behaviors. I’ve already done most of the preparations. User objects (and their kin) now have a preference that specifies which render engine to use and preference strings for the templates. User.file_naming_template_class will return the name of the class to use. I’ll spare you the eyes-bleeding cross product of all the different variations, here’s my first cut at the new factory class.

[ick] but, you know, make it work, then make it right… 

class FileNameRenderer
  include Singleton

  class << self
    def factory(object, actor, template_name)
      template = actor.try(template_name) || default(FileNaming, template_name)
      FileNaming.new(object, template)
    end

    def default(klass, template)
      if klass == FileNaming
        default_file_naming(template)
      else
        fail ArgumentError, klass
      end
    end

    def default_file_naming(template)
      case template
      when :notification_filename_template then FileNaming::TRANSMOGRIFIED_TEMPLATE
      when :document_filename_template then FileNaming::DOCUMENT_TEMPLATE
      else
        fail ArgumentError, template
      end
    end
  end
end

Let’s move all of the default behavior into the FileNaming class.

class FileNaming
  # ...
  def default(template)
    case template
    when :notification_filename_template then FileNaming::TRANSMOGRIFIED_TEMPLATE
    when :document_filename_template then FileNaming::DOCUMENT_TEMPLATE
    else
      fail ArgumentError, template
    end
  end
end

And a little more refactoring so that the renderer class is computed:

class FileNameRenderer
  include Singleton

  class << self
    def factory(object, actor, template_name)
      klass = renderer_class(actor)
      template = actor.try(template_name) || klass.default(template_name)
      klass.new(object, template)
    end

    def renderer_class(actor)
      FileNaming
    end
  end
end

This should actually work with my existing code base, so now I’m going to change all of the places where FileNaming objects were created directly and replace it with this factory pattern. Status update: Gotta go make dinner for the kids. I found some leaks in our abstractions. Sometimes our code references the template name (or method) and sometimes we just mess around with the template content (as a String). Primitive Obcession bites us, again.

While I was gone , I shipped the code into production. Nothing changed. Hurray! Now I can make some small changes to use our new rendering class. The “actor” object has a preference setting that knows which class to use. (I’ve made a slight hack for the null object smell. Sometimes a little bit of deodorant…)

class FileNameRenderer
  # ...
  class << self
    def renderer_class(actor)
      actor&.file_naming_template_class || FileNaming
    end
  end
end

There’s some magic going on behind the scenes — the Preferences module also disambiguates between template systems, but that’s another beast of code. (get_preference reaches into the stored data to get the attribute with that name)

def document_filename_template
  case get_preference('file_naming_template_class')
  when 'FileNaming' then get_preference('document_filename_template')
  when 'FileNameTemplate' then get_preference('document_filename_template_v2')
  else
    fail ArgumentError, file_naming_template_class
  end
end

What I didn’t show you was that code moving around from the template system to the preferences system. And that’s okay, the tests guided me and when I noticed duplicate code and feature envy, it was an “obvious” move to make. Making mistakes is okay. Just get it working. I didn’t write a whole lot of tests, either. Just the behavior I wanted. That way, when I refactored, I didn’t have to rewrite the tests.

The tests are ‘two’ lines long: Act and Assert (the middle line is mostly a sanity check that I’m using the correct template class). I’m doing all of the Arranging in a before block. See also Arrange Act Assert 

context "using the FileNameTemplate renderer" do
  before do
    user.file_naming_template_class = 'FileNameTemplate'
  end

  it "renders using the Liquid template, instead" do
    renderer = FileNameRenderer.factory(object, user, :document_filename_template)
    expect(renderer).to be_a(FileNameTemplate)
    expect(renderer.document_filename('.pdf')).to eq("N.D. Cal. 15-cv-37731 dckt 000094_002 filed 2017-06-13.pdf")

    renderer = FileNameRenderer.factory(object, user, :notification_filename_template)
    expect(renderer).to be_a(FileNameTemplate)
    expect(renderer.document_filename('.pdf')).to eq("2017-06-13 MOTION FEE SERVICE [dckt 94_2].pdf")
  end
end

Remember the default value code I wrote for the FileNaming class? Here’s the same exact code for the FileNameTemplate class. The only differences are the values of constants. Here, code duplication is an indication that we’re on the right track.

class FileNameTemplate
  # ...
  def default(template)
    case template
    when :notification_filename_template then TRANSMOGRIFIED_TEMPLATE
    when :document_filename_template then DOCUMENT_TEMPLATE
    else
      fail ArgumentError, template.inspect
    end
  end
end

Time for another pull request and a deploy to production!

That’s about it. Now we can reconfigure a User (or an Organization an Office) to switch rendering systems and then they will use the new Liquid templates from Preferences to render. Existing customers are unaffected until then. For all of the users that are still using our default templates, we can bulk switch them over.

Live Refactoring - April 28, 2019 - Ken Mayer