Skip to main content
Let’s get started!
View all Services
Development
Design
Product
Team and Processes
View all Services
View all Resources
Development
The business of great software
View all Resources

Refactoring: Replace Conditional with Polymorphism

Joe Ferris

I recently discovered the validates! method in ActiveModels’ validations API. To paraphrase the official documentation:

This method is used to define a validation that cannot be corrected by the
end user and is considered exceptional. Each validator defined with bang or
the :strict option set to true will raise
ActiveModel::StrictValidationFailed instead of adding an error when the
validation fails.

This method was exactly what I had been looking for, so I set to using it with gusto. Of course, before I gusted out a quick validates! call, I went to write a failing test:

describe AccessRequest do
  it { should allow_value('Pending', 'Rejected', 'Accepted').for(:status) }
  it { should_not allow_value('Pancake').for(:status) }
end

These tests work fine for the usual validates_inclusion_of approach, but there was nothing I could add to make them work with the :strict option, since allow_value doesn’t expect exceptions. Since shoulda-matchers is open source, I rolled up my sleeves and jumped in. My tests now worked with the simple addition of a strict call to the matcher:

describe AccessRequest do
  it { should allow_value('Pending', 'Rejected', 'Accepted').
         for(:status).strict }
  it { should_not allow_value('Pancake').
         for(:status).strict }
end

What’s that smell

Everything was working great, but my change had introduced a terrible smell: AllowValueMatcher now had six methods that forked their behavior based on whether or not strict? was set. This had a number of downsides:

  • Maintainability: we now had two ways of looking for error messages: from the errors collection, and from an exception raised during validation. This commit makes it clear that at least six places need to change if we add any more.
  • Maintainability: this commit revealed that AllowValueMatcher had too many concerns: it both finds error messages to check, and parses the options and errors to determine whether or not the errors were expected.
  • Readability: AllowValueMatcher is now too large to understand at a glance, weighing in at 26 methods.
  • Readability: having two logical paths for every method resulted in a lot of long method names. In addition, having so many methods makes it more difficult to reduce the vertical distance between a method and the methods that reference it.
  • Testability: making sure that the various forks work as expected requires testing everything in AllowValueMatcher twice. There will always be several methods in between the test harness and the method under test, making the test more difficult to write, understand, and debug.

I had a fever, and the only cure was more objects. The prescription: a healthy dose of Replace Conditional with Polymorphism.

The skinny

The full change I pushed to solve this problem is fairly large:

This improves the code in a few ways:

  • Maintainability: adding another way to find error messages just requires implementing the implicit MessageFinder interface.
  • Readability: the logic for finding messages of a particular type is now encapsulated in a small class per type, reducing the logical distance between methods and references and giving each method a clearer logical context.
  • Testability: the method under test can now be called directly from the test harness.

The problem and results are fairly easy to understand, but let’s dig into the process of cleaning up this mess.

One branch at a time, one method at a time

This is a higher level refactoring that ideally consists of many, small refactorings. I started with one logical branch: finding errors from the errors collection.

I created a new method just for the logical branch I was changing:

def description
  if strict?
    "strictly #{allow_description(allowed_values)}"
  else
    allow_description(allowed_values)
  end
end

private

def allow_description(allowed_values)
  "allow #{@attribute} to be set to #{allowed_values}"
end

This change is known as Extract Method. This refactoring didn’t change the API at all and required no test changes. Next, I added a reference to the object I planned on providing:

def description
  if strict?
    "strictly #{message_finder.allow_description(allowed_values)}"
  else
    message_finder.allow_description(allowed_values)
  end
end

This fails because message_finder isn’t defined. I defined that as a hard-coded reference to the first implementation I planned to write:

def message_finder
  @message_finder ||= ValidationMessageFinder.new(@instance, @attribute)
end

Now I had a failure because that class was undefined. I added my first test:

context '#allow_description' do
  it 'describes its attribute' do
    model_class = define_model(:example, :attr => :string)
    instance = model_class.new
    finder = Shoulda::Matchers::ActiveModel::ValidationMessageFinder.new(
      instance,
      :attr
    )

    description = finder.allow_description('allowed values')

    description.should == 'allow attr to be set to allowed values'
  end
end

The failures from this test guided me to define the class and implement the initialize method. The last change was to move the original allow_description method from AllowValuesMatcher to my new class (Move Method). That change caused my new test to pass. At this point, all tests for AllowValueMatcher also passed.

The process of extracting and moving a method is known as Extract Class. I continued to extract and move methods one at a time until every non-strict branch in a conditional was delegated to the message finder. This change resulted in my first commit to replace the conditionals.

At this point, I had extracted a new class, but all six conditionals remained. I wanted to move the other half of each branch into another new class, so I needed a way to get the new class into the mix.

I changed AllowValueMatcher to start out with a ValidationMessageFinder:

def initialize(*values)
  @values_to_match = values
  @strict = false
  @message_finder_factory = ValidationMessageFinder
  @options = {}
end

I changed AllowValueMatcher#strict to replace the finder factory:

def strict
  @message_finder_factory = ExceptionMessageFinder
  @strict = true
  self
end

And I changed AllowValueMatcher#message_finder to use the factory:

def message_finder
  @message_finder ||= @message_finder_factory.new(@instance, @attribute)
end

Then I repeated this process of extracting a class and moving extracted methods one at a time, this time for the strict branch of each conditional. Each replaced method in this second pass went through three stages:

# Original method
def description
  if strict?
    "strictly #{message_finder.allow_description(allowed_values)}"
  else
    message_finder.allow_description(allowed_values)
  end
end

# Extract positive branch to a method and move it to `ExceptionMessageFinder`
def description
  if strict?
    message_finder.allow_description(allowed_values)
  else
    message_finder.allow_description(allowed_values)
  end
end

# Both branches are the same. Simplify!
def description
  message_finder.allow_description(allowed_values)
end

After removing all six conditionals, the @strict attribute and strict? method were no longer necessary, so I removed them. At this point, I made my second commit, completing my higher-level refactoring.

Putting it all together

This refactoring involved many steps, but each step was small, easy to follow, and left me with a functioning system and passing tests. Improving code using many small steps (refactoring) is faster and less error-prone than trying to change a lot of code in one large step (rewriting). I also made frequent commits during this process so that I could easily step backwards if any of my steps were a mistake. Before pushing this, I rebased and squashed most of my commits together.

I finished it up with the inevitable fix for Ruby 1.8.7 and put together a pull request to get feedback from other developers.

Next time you’re unhappy with your code, resist the temptation to rip it out and write it fresh, even if you’re only replacing a single class or a few methods. Try using some of the small steps described above for a safer, less stressful coding session.

Detect emerging problems in your codebase with Ruby Science. We’ll deliver solutions for fixing them, and demonstrate techniques for building a Ruby on Rails application that will be fun to work on for years to come.

Grab a free sample of Ruby Science today!