giant robots smashing into other giant robots

We are thoughtbot. We make web & mobile apps.

Tagged:

Comments (View)

Inject that Rails Configuration Dependency!

Setting up configuration settings in a Rails app has been fairly straightforward for a while now:

# config/environments/development.rb
Doit::Application.configure do
  config.default_creator = "Person 1"
end

# config/environments/test.rb
Doit::Application.configure do
  config.default_creator = "Test Person"
end

# config/environments/production.rb
Doit::Application.configure do
  config.default_creator = "John Doe"
end

To access this setting, call Doit::Application.config.default_creator within your app. Pretty straightforward, right?

class TodoItem < ActiveRecord::Base
  before_create :assign_default_creator, unless: :creator?

  private

  def assign_default_creator
    self.creator = Doit::Application.config.default_creator
  end
end

Let’s imagine you have a todo item and you want to test that the value gets assigned if no creator is provided.

Imagine how you’d test this.

describe TodoItem do
  it "assigns the default creator when no creator is assigned" do
    Doit::Application.config.stub(:default_creator).and_return("default creator from config")
    subject.save
    subject.creator.should == "default creator from config"
  end

  it "does not assign the default creator if it has been set" do
    Doit::Application.config.stub(:default_creator).and_return("default creator from config")
    subject.creator = "Jane Doe"
    subject.save
    subject.creator.should == "Jane Doe"
  end
end

There’s a few things that are gross. First, we’re referencing Doit::Application in the model spec. Second, we’re stubbing in both examples. The latter is a low-hanging fruit so it can be extracted.

describe TodoItem do
  it "assigns the default creator when no creator is assigned" do
    config_default_creator_returns("default creator from config")
    subject.save
    subject.creator.should == "default creator from config"
  end

  it "does not assign the default creator if it has been set" do
    config_default_creator_returns("default creator from config")
    subject.creator = "Jane Doe"
    subject.save
    subject.creator.should == "Jane Doe"
  end

  def config_default_creator_returns(value)
    Doit::Application.config.stub(:default_creator).and_return(value)
  end
end

Now, interacting with Doit::Application is confined to one method. Some people may stub the application in an RSpec before block, but I don’t like doing that because one of the specs cares about the stubbed value. I want that stub right in the example so it’s obvious that the stub and assertion are close (in number of lines).

Even though Doit::Application is confined to one call in the spec, I really don’t like that the spec cares about its config at all. What I’d love to do is assign a custom configuration on my TodoItem in my test intsead of caring about Doit::Application and having to stub on config. I can do this with dependency injection.

Right now, TodoItem has a dependency on Doit::Application.config. Dependency injection would mean TodoItem gets a class_attribute :config that defaults to Doit::Application.config but can be overridden (say, in our tests).

describe TodoItem do
  it "assigns the default creator when no creator is assigned" do
    subject.config = stub("config", default_creator: "default creator from config")
    subject.save
    subject.creator.should == "default creator from config"
  end

  it "does not assign the default creator if it has been set" do
    subject.config = stub("config", default_creator: "default creator from config")
    subject.creator = "Jane Doe"
    subject.save
    subject.creator.should == "Jane Doe"
  end
end

With the class attribute, I’m able to override config on the instance and replace it with a stub that has a default_creator method, which I’ve assigned to the string I expect. I was able to remove my reference of Doit::Application from the spec. Perfect!

Here’s the model code:

class TodoItem < ActiveRecord::Base
  class_attribute :config
  self.config = Doit::Application.config

  before_create :assign_default_creator, unless: :creator?

  private

  def assign_default_creator
    self.creator = config.default_creator
  end
end

The callback assign_default_creator now doesn’t care about Doit::Application.config, only that config has a default_creator method.

This post actually stemmed from my interaction with a developer in Factory Girl GitHub Issues who asked a pretty interesting question about reloading classes in Factory Girl after removing the constant and loading the Ruby file again (it seemed like a code smell and not an issue with Factory Girl).

At the end of the thread, I suggested he attend my Test-Driven Rails workshop next week, January 30th and 31st, because I’ll be talking about RSpec and dependency injection (among other things like Cucumber, how, when, and what to test in a Rails app). It’s perfect for Rails developers who are interested in writing more cleaner, more stable applications.

See you there!

Tagged:

Comments (View)

A Paperclip Refactoring Tale: Part One: Dependency Injection

The Beginnings

Good refactoring has a goal. The goal of this refactoring is to make it easier for people to hook into Paperclip.

To start I surveyed existing gems, plugins, and apps that hook into Paperclip. I took a quick glance through Github and found that delayed_paperclip, mongoid-paperclip, papermill, paperclip-extended, and paperclip-facecrop all served as good sample gems that change Paperclip in unique ways. My end desire was to remove the need for monkey patching, and if I could go one further to remove the need for subclassing that’d be even better.

Papermill is a sample Rails app that uses Paperclip, extending it with watermarking, copyrighting, and a custom geometry string. It does this by extending the existing thumbnail Paperclip processor, overriding two methods: the constructor and transformation_command. The new constructor does a ton of parsing: the geometry string is parsed for options indicating a desire to add a copyright or watermark, the copyright or watermark string is extracted from the options, and cropping offsets are parsed out too. It uses all of this to manipulate the transformation_command, like this: it invokes the super’s transformation_command, which produces a string. It then subs the string, sticking various bits set from the constructor into the middle of the command. The watermarking aspect is particularly tricky, since it must change the convert command to a convert ... | composite command, using string manipulation.

This will not do.

The Abstractions

The first change I made was to parameterize the geometry parsing (3f7aee3 and eebc7d9). Now if you want a Thumbnail-based processor but with a custom geometry parser you can do that right from has_attached_file:

has_attached_file :avatar,
  :styles => {:medium => {
    :geometry => '80x60#10x5:2x1',
    :string_geometry_parser => CroppingGeometryCommandParser.new
  }}

This change looks a little like this. Before:

module Paperclip
  class Thumbnail < Processor
    def initialize(file, options = {}, attachment = nil)
      super

      geometry             = options[:geometry] # this is not an option
      @file                = file
      @target_geometry     = Geometry.parse(geometry)
      @current_geometry    = Geometry.from_file(@file)
    end 
  end
end

After:

module Paperclip
  class Thumbnail < Processor
    def initialize(file, options = {}, attachment = nil)
      super

      geometry             = options[:geometry] # this is not an option
      @file                = file
      @target_geometry     = (options[:string_geometry_parser] || Geometry).parse(geometry)
      @current_geometry    = (options[:file_geometry_parser] || Geometry).from_file(@file)
    end 
  end
end

The change? Instead of hardcoding the Geometry class it instead takes it as an argument. The geometry parsing can now be replaced by any arbitrary object. In the tests, for example, instead of creating files that parse to the proper size or instead of stubbing, you can just pass an object that has a #from_file method and produces the correct object. The Geometry class is still the default for legacy reasons.

I split these into the string_geometry_parser and file_geometry_parser because they do different tasks: one takes a file and produces an object that knows how to produce the desired scaling and cropping arguments (lets call this the transformation), and another produces an object that is passed to the transformation.

With that small but powerful change out of the way I took a look at paperclip-extended. This is an old gem; many of these extensions have been rolled into Paperclip by now. The only thing that was impossible without monkey patching was the extended interpolations. For example, normally you can have a file path like /system/images/:rails_env/:id/i:basename.:extension and the :rails_env, :id, :basename, and :extension will be replaced with proper values—but it was impossible to add new interpolation values, such as :normalized_basename (removing non-alphanumerics).

The fix for this (7478455) is similar to that for geometry parsing: pass a custom interpolator to the Attachment through has_attached_file:

has_attached_file :avatar,
  :interpolator => InterpolatorWithNormalization.new

This change takes place in the Attachment class. Before:

module Paperclip
  class Attachment
    def interpolate(pattern, style_name = default_style)
      Paperclip::Interpolations.interpolate(pattern, self, style_name)
    end
  end
end

After:

module Paperclip
  class Attachment
    def initialize(name, instance, options = {})
      @interpolator = (options[:interpolator] || Paperclip::Interpolations)
    end

    def interpolate(pattern, style_name = default_style)
      @interpolator.interpolate(pattern, self, style_name)
    end
  end
end

Again, a straight-forward change that makes this class easier to test and also adds more extensibility.

The Reasonings

This is dependency injection. It is a very boring and simple principle that goes like this: abstract out class names where possible. This gains you both ease of testing and straight-forward hooks for extensibility.

You just saw two examples of it from a real project, so I’ll follow it up with an example from nothing:

Before:

class Car
  def gas
    puts "Vroom putter putter putter"
  end

  def brake
    puts "screeeetch!"
  end
end

After:

class Car
  attr_accessor :engine

  def initialize(engine)
    @engine = engine
  end

  def gas
    engine.go!
  end

  def brake
    engine.stop!
  end
end

This gains you testing ease:

describe Car do
  let(:engine) do
    Class.new do

      def go!
        @has_gone = true
      end

      def stop!
        @has_stopped = true
      end

      def has_gone?
        @has_gone
      end

      def has_stopped?
        @has_stopped
      end

    end.new
  end

  subject { Car.new(engine) }

  it "goes" do
    car.gas
    engine.should have_gone
  end

  it "stops" do
    car.brake
    engine.should have_stopped
  end
end

… and flexibility:

yuppie = Person.new(:car => Car.new(HybridEngine.new(:diesel => 40, :electricity => 60))
bro = Person.new(:car => Car.new(V12Engine.new(:mpg => 2))

Y’all

Now you get to use this. Things you can do:

  • Write a Gem that provides a custom Paperclip interpolator. Encrypt file names, handle uppercase/lowercase issues, offer the time of day, offer a random number, and so on—anything you need!
  • Write a gem that provides a custom Paperclip geometry sizer. It scales and crops images differently. For example, pick out people’s faces, crop from the center, measure hard-to-measure images, and so on.
  • While refactoring your code use a dependency injection principle somewhere. Instead of Net::HTTP use a passed-in object; same for any credit card processing, Tweeting, file-writing, logging, and so on.