She Blinded Me with Ruby Science

We love Rails, object-oriented programming, and refactoring. We use a process to develop applications to work faster, introduce fewer bugs, and enjoy what we’re doing. We blog, Tweet, and talk at conferences on these subjects.

After every post and discussion, there are topics left unexplored. There are too many related concepts to link to, and there’s too much code we can’t share. We want to share our process with as many Rails developers as we can, and we’ve finally decided to take the next step. We’re writing a new book, called Ruby Science, that describes this process in an easy-to-digest reference format.

But this is more than just a book. As with Backbone.js on Rails, in addition to the book (in HTML, PDF, EPUB, and Kindle formats), you also get a complete example application, and the ability to get your questions about Ruby on Rails answered by the thoughtbot team using GitHub issues and live chat.

The book is written using Markdown and pandoc and distributed via GitHub. When you purchase, we give you direct access to the repository, so you can use the GitHub comment and issue features to give us feedback about what we’ve written and what you’d like to see. Give us your toughest Ruby questions with GitHub issues, and we’ll answer. Last but not least, also included is a Ruby on Rails reference application. What the book describes and explains, the example app demonstrates with real, working code. Fully up to date for Rails 3.2.

The book is a work in progress, and we currently have almost 40 pages of content. You can get access now for an early purchase price of $39. This offer is only valid for a limited time, and will increase to $49 on January 31, 2013.

You can see our working table of contents, read a free sample, and purchase access on the Ruby Science web page.

Let’s not

RSpec is an excellent test framework with a large community and an active team of maintainers. It sports a powerful DSL that can make testing certain things much easier and more pleasant.

However, there are a few features of RSpec’s DSL that are frequently overused, leading to an increase in test maintenance and a decrease in test readability.

Let’s look at an example from factory_girl’s test suite and see how we can improve it by favoring plain Ruby methods over DSL constructs:

describe FactoryGirl::EvaluatorClassDefiner do
  let(:simple_attribute) {
    stub("simple attribute", name: :simple, to_proc: -> { 1 })
  }
  let(:relative_attribute) {
    stub("relative attribute", name: :relative, to_proc: -> { simple + 1 })
  }
  let(:attribute_that_raises_a_second_time) {
    stub(
      "attribute that would raise without a cache",
      name: :raises_without_proper_cache,
      to_proc: -> { raise "failed" if @run; @run = true; nil }
    )
  }
  let(:attributes) {
    [
      simple_attribute,
      relative_attribute,
      attribute_that_raises_a_second_time
    ]
  }
  let(:class_definer) {
    FactoryGirl::EvaluatorClassDefiner.new(
      attributes,
      FactoryGirl::Evaluator
    )
  }
  let(:evaluator) {
    class_definer.evaluator_class.new(
      stub("build strategy", add_observer: true)
    )
  }

  it "adds each attribute to the evaluator" do
    evaluator.simple.should eq 1
  end

  it "evaluates the block in the context of the evaluator" do
    evaluator.relative.should eq 2
  end

  # More tests
end

A General Fixture is declared at the top. The fixture is then reused and augmented by each test to create the necessary setup. The examples (the it blocks) don’t declare any test setup; instead, they reference relevant portions of the existing fixture.

This approach causes a number of issues:

• It obscures each test by introducing a Mystery Guest.
• It causes Fragile Tests by creating a complicated fixture that is difficult to maintain.
• It causes Slow Tests by creating more data than is necessary in each test.

Will our mystery guest please leave

Addressing the Mystery Guest issue solves the largest concern: readability. A mystery guest causes obscure tests. Gerard Meszaros defines the Mystery Guest in his xUnit Patterns:

The test reader is not able to see the cause and effect between fixture and
verification logic because part of it is done outside the Test Method.

Here are some examples from this test suite:

it "adds each attribute to the evaluator" do
  evaluator.simple.should eq 1
end

it "evaluates the block in the context of the evaluator" do
  evaluator.relative.should eq 2
end

Without context, the reader has no idea what’s happening in this test, and the example description can’t really help. By parsing out the large fixture above, the reader can determine what’s going on, but correlating the fixture and test is slow and error-prone.

Let’s start by in-lining the fixture for this example:

it "evaluates the block in the context of the evaluator" do
  simple_attribute =
    stub("simple attribute",   name: :simple, to_proc: -> { 1 })
  relative_attribute =
    stub("relative attribute", name: :relative, to_proc: -> { simple + 1 })
  attribute_that_raises_a_second_time =
    stub("attribute that would raise without a cache",
         name: :raises_without_proper_cache,
         to_proc: -> { raise "failed" if @run; @run = true; nil })

  attributes = [
    simple_attribute,
    relative_attribute,
    attribute_that_raises_a_second_time
  ]
  class_definer = FactoryGirl::EvaluatorClassDefiner.new(
    attributes,
    FactoryGirl::Evaluator
  )
  evaluator = class_definer.evaluator_class.new(
    stub(
      "build strategy",
      add_observer: true
    )
  )

  evaluator.simple.should eq 1
end

The test continues to pass. Looking through the expected result, we can see that some data isn’t actually used in this scenario. Let’s remove it:

it "adds each attribute to the evaluator" do
  simple_attribute =
    stub("simple attribute",   name: :simple, to_proc: -> { 1 })

  attributes =
    [simple_attribute]
  class_definer = FactoryGirl::EvaluatorClassDefiner.new(
    attributes,
    FactoryGirl::Evaluator
  )
  evaluator = class_definer.evaluator_class.new(
    stub("build strategy", add_observer: true)
  )

  evaluator.simple.should eq 1
end

Now let’s in-line the fixture and remove unrelated data for the second example:

it "evaluates the block in the context of the evaluator" do
  simple_attribute =
    stub("simple attribute",   name: :simple, to_proc: -> { 1 })
  relative_attribute =
    stub("relative attribute", name: :relative, to_proc: -> { simple + 1 })

  attributes =
    [simple_attribute, relative_attribute]
  class_definer = FactoryGirl::EvaluatorClassDefiner.new(
    attributes,
    FactoryGirl::Evaluator
  )
  evaluator = class_definer.evaluator_class.new(
    stub("build strategy", add_observer: true)
  )

  evaluator.relative.should eq 2
end

Now that we’ve in-lined these two fixtures, there’s obviously a lot of duplicated setup logic. Let’s extract all that to a few factory methods:

it "adds each attribute to the evaluator" do
  attribute = stub_attribute(:attribute) { 1 }
  evaluator = define_evaluator(attributes: [attribute])

  evaluator.attribute.should eq 1
end

it "evaluates the block in the context of the evaluator" do
  dependency_attribute = stub(
    "dependency",
    name: :dependency, to_proc: -> { 1 }
  )
  dependency_attribute = stub_attribute(:dependency) { 1 }
  attribute = stub_attribute(:attribute) { dependency + 1 }
  evaluator = define_evaluator(
    attributes: [dependency_attribute, attribute]
  )

  evaluator.attribute.should eq 2
end

def define_evaluator(arguments = {})
  evaluator_class = define_evaluator_class(arguments)
  evaluator_class.new(FactoryGirl::Strategy::Null)
end

def define_evaluator_class(arguments = {})
  evaluator_class_definer = FactoryGirl::EvaluatorClassDefiner.new(
    arguments[:attributes] || [],
    arguments[:parent_class] || FactoryGirl::Evaluator
  )
  evaluator_class_definer.evaluator_class
end

def stub_attribute(name = :attribute, &value)
  value ||= -> {}
  stub(name.to_s, name: name.to_sym, to_proc: value)
end

Once we convert the remaining examples, we can delete the let statements that created the general fixture.

And the winner is…everyone

These converted examples are greatly improved:

• They’re easier to read, because all the actors referenced from the verification step are declared in the setup step within the it block.
• They’re less brittle, because each example only specifies the information it needs.
• They’re faster, because each example is running with a smaller data set.

It turns out that removing the Mystery Guests also solved our other complaints with these tests.

An added benefit is that the factory methods we created are easier to reuse throughout the test suite, whereas let statements are too specific to the examples for each example group. In time, this approach will make the entire test suite easier to maintain.

Until you need to break out the big guns like shared examples, avoid DSL constructs like subject, let, its, and before. Stick to your old friends: variables, methods, and classes.

Next Steps & Related Reading

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!

Refactoring: Replace Conditional with Polymorphism

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:

• I moved all of the error finding and description functionality into two new classes: ValidationMessageFinder and ExceptionMessageFinder.

• The matcher is assigned a message finder factory based on whether or not it’s strict.

• Each method that used to have a conditional now delegates to its message finder.

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.

Next Steps & Related Reading

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!

Using Writing Smells to Refactor Your Email

I’ve recently started applying some code refactoring techniques to writing emails. Yes, emails.

Your goal as the writer

1) Get the reader to read the most important thing
2) Get them to respond quickly or do something quickly

We all get too much email and the following is my approach to try and make emails simpler, easier to respond to, and more readable.

I adapt the concept of code smells for use in email as “writing smells”. Here’s my process and an example. It’s not perfect but it’s useful for me.

Writing Smells

Forest for the Trees

Break every sentence into a separate paragraph (each on a new line with a blank between). This helps to highlight long sentences. Also, it narrows your refactoring focus to one sentence at a time and improves readability.

More Than One “and”

Break long sentences down into shorter ones, especially if they have two ideas/goals. Example offender:

“If you haven’t already, take a look at our site http://thoughtbot.com for some examples of projects we’ve done and our playbook (http://playbook.thoughtbot.com) for more detail on our process and approach to building products”

Sentences > 20 Words

Start at the top and focus on one sentence at a time. Play Sentence Golf - use as few words as possible and still be clear.

Now What?

Your goal as an email sender is to convey information and get some kind of action to take place. If someone reads your message and it doesn’t address what they should do, it’s going to get ignored.

First Sentence Doesn’t Convey Everything

The most important thing for the reader to pay attention to should be at the top. Most email readers only get to the first sentence - make it count.

Open-ended Choices

You should offer the recipient only one decision and it should be a “yes/no” choice. If your question requires work, you have a lower chance of getting a response. For example, when proposing to meet up, say

“I like Cafe Madeline on New Montgomery. Let’s meet there Tuesday at 4pm.”

instead of

“Where should we meet?”

If they don’t like the date/time, they will propose something else.

More than 5 Sentences

Keep it to 5 sentences or fewer if you can.

Meh

Throw a joke in there. Email sucks and we have tons of it. Amuse your reader somehow.

And I Know You From…?

If it’s your first message to someone, demonstrate how you know each other, “We met at last night’s meetup”. Throw in some connection like “We are both from Quebec!”

Kinda Sorta

Avoid being wishy washy. Be direct. For example,

“If you’re still going to be in town next week I’d be glad to meet with you and talk about what you guys are doing at example.com, what we do here at thoughtbot and how we could help you out.”

becomes

“If you’re in town next week, let’s meet up. I suggest Cafe Madeline…”

Overuse

I overuse the word ‘that’. When finishing an email, I do a Ctrl-F on ‘that’ and either delete it or replace it with ‘which’ when appropriate. Or use a tool to find words you tend to overuse.

Like the Plague

Avoid cliches. Like the plague.

Here’s an example

Before:

“Hey Ralph,

I’m glad I got to meet you last night. If you’re still going to be in town next week I’d be glad to meet with you and talk about what you guys are doing at example.com, what we do here at thoughtbot and how we could help you out. If you haven’t already, take a look at our site http://thoughtbot.com/ for some examples of projects we’ve done and our playbook (http://playbook.thoughtbot.com/) for more detail on our process and approach to building products. If you are interested I can also send you a document which provides an overview of our process. Looking forward to hearing from you. -Rolf”

After:

“Hey Ralph,

I’m glad to meet another fellow Quebecois and lover of poutine last night at the meetup.

If you’re in town next week, let’s meet up. I suggest Cafe Madeline for coffee on Tuesday at 3pm.

I’m interested to learn more about example.com and tell you more about thoughtbot.

If you haven’t already done so, take a look at thoughtbot.com to see some of our recent projects.

Also, take a look at our playbook (playbook.thoughtbot.com) for more detail on our approach to building startups.

Looking forward to hearing from you.

-Rolf”

Further reading:

• Guy Kawasaki’s “The Effective Emailer”
• Shell Scripts to Improve Writing

Photo: Skunk Encounter by vladeb, on Flickr

How much should I refactor?

The Rails community has been abuzz with object-oriented programming, SOLID principles, laws, design patterns, and other principles, practices, and patterns. We’ve (re)discovered new tools and techniques to separate and reuse logic, making code easier to test, understand, and maintain. Now that we’ve learned about all these new tools, when do we use them?

Going too far

Applying design practices correctly takes a lot of practice, and when you first discover a hammer, everything looks like a nail. You can refactor every class ad-nauseum until each class obeys every letter of the SOLID principles, but that approach will hurt you: for one, not every class will ever see too much use or change, but for another, many of the abstractions advocated by those principles can hurt overall readability in the short term. Extracting a method means you need to jump around a file to figure out what’s going on. Extracting a class means you have to jump between files. Extracting a library means you need to jump between projects. Adding names can clarify what you’re doing, but adding too many names results in a vocabulary overload. Abstractions can grant you infinite flexibility but zero readability.

Getting a sour taste

If you’ve seen these principles applied incorrectly or overapplied, you may be tempted to throw the baby out with the bath water. Many Ruby developers came from Java, and after a few years of working with AbstractUserDecoratorFactoryFactories, some of them decided to never use the words “design pattern” again. Many principles have been applied with good intention and terrible results, but that’s no reason to throw out everything we’ve learned since the inception of object-oriented programming.

Applying the Dependency Inversion Principle in one situation may create an unreadable, abstract puzzle, but using it when you need it can keep a single class from ruining an entire application. Applying any principle as a black and white law will result in a game of refactoring whack-a-mole. However, if we can’t apply these principles universally, how do we know when to be aggressive about refactoring?

God classes

Every project has them: one or two classes that seem to know everything. Any question you could ask in the domain is answered by one of these classes. Any class you look into seems to depend on them. You can’t change any class in the system without breaking the tests for a god class. My experience has taught me that most projects will have two god classes: User and whatever the focus happens to be for that application. In a blog application, it will be User and Post. In an Agile project management tool, it will be User and Story. God classes grow and become entangled with every other component of the system until there’s no room to breath.

God classes are a great place to start aggressive refactoring. Without even looking inside their files, you can probably tell which two classes in your project are the culprits. I think very carefully before adding any behavior to these classes. I apply SOLID principles rigourously to these classes. If I can extract behavior from one of these classes using a design pattern like observer or decorator, I’ll do it.

Extracting behavior from a class like this doesn’t reduce overall comprehensibility, because the class is already too large to comprehend. Introducing abstractions to these classes makes them easier to understand, because you can at least fit all the abstractions in your head at once, even if you can’t remember what the implementations are like.

Churn

You can use tools like Churn to figure out which files in your project change the most. The top two contenders are likely to be your project’s god classes, but you may be surprised which other files change the most. If something changes once, it’s likely to change again, and locating the pieces of these classes that change and extracting them will make the next change faster. Refactor the classes that change the most by slimming them down and making them as readable as possible to increase productivity and help to avoid defects.

Bugs

If you can pinpoint a bug to a particular class, it’s likely that refactoring that class will help to prevent the next bug. Bugs love company, and the same parts of an application tend to break again and again. Extractions can help isolate the trickier components, and refactoring may reveal that the bugs are cropping up because you were thinking about the problem the wrong way to begin with. Keep track of which files you change when fixing bugs; these files are great targets for aggressive refactoring.

Crossing the finish line

When do you stop refactoring? When is a change good enough to commit? When is a branch good enough to deploy? When is a library good enough to release?

A program is never finished, and no amount of refactoring will make it perfect. I refactor to fix the parts of an application or library that have caused me pain. Rather than attacking theoretical problems, identify components that have actually bitten you and apply theory to tame them. Refactor as you go, and fix one problem at a time.