Taking DRY Too Far

Taking DRY too far is easy. A line of code appearing identically in two places in a class or codebase does not necessarily mean the duplication needs to be removed. Here is an example:

class PeopleController < ApplicationController
  def list
    @people = Person.find(:all)
  end
  def show
    @person = Person.find(params[:id])
  end
  def destroy
    @person = Person.find(params[:id])
    @person.destroy
    redirect_to people_url
  end
end

An over-zealous developer may look at this snippet and see the first line in each action is duplicated. He or she might do this:

class PeopleController < ApplicationController
  def list
    @people = Person.find(:all)
  end

  def show
    @person = find_person
  end

  def destroy
    @person = find_person
    @person.destroy
    redirect_to people_url
  end

  protected

  def find_person
    Person.find(params[:id])
  end
end

Adding the find_person method is wrong. This is not what the DRY principle is about. Also, the show and destroy actions still have the same duplication, just in a different way. This change could also hurt maintainability. The show and destroy actions may want to find the person in a slightly different way. That may sound extremely hypothetical, but a scenario is if the show action should only display certain records, while any record can be deleted. Perhaps to remove the calls to find a developer could do this:

class PeopleController < ApplicationController
  before_filter :find_person, :only => %w[show destroy]

  def list
    @people = Person.find(:all)
  end

  def show
  end

  def destory
    @person.destory
    redirect_to people_url
  end

  protected
  def find_person
    @person = Person.find(params[:id])
  end
end

Even though that duplicated line of code is gone, we're now abusing filtering (if you disagree, I'll save details on that for another post). Finding the person to use in the action should not be done in a filter that runs before the action - it belongs in the action. The first code snippet in this post is the best - no before_filter, no private method.

Here is a good example of a code that actually violates DRY.

class SimpleCalculator
  def initialize(verbose = true)
    @verbose = verbose
  end
  
  def add(first_number, second_number)
    result = first_number + second_number
    puts "#{first_number} + #{second_number} is #{result}" if @verbose
    result
  end
  
  def substract(first_number, second_number)
    result = first_number - second_number
    puts "#{first_number} - #{second_number} is #{result}" if @verbose
    result

  end
end

Notice how both of the "puts" lines end with "if @verbose". This violates DRY; it hurts maintainability. Every time we add a "puts" in this class, we need to tack on "if @verbose". Here is a much better solution:

class SimpleCalculator
  def initialize(verbose = true)
    @verbose = verbose
  end
  
  def add(first_number, second_number)
    result = first_number + second_number
    say "#{first_number} + #{second_number} is #{result}"
    result
  end

  
  def substract(first_number, second_number)
    result = first_number - second_number
    say "#{first_number} - #{second_number} is #{result}"
    result
  end

  
  protected
  
  def say(message)
    puts message if @verbose
  end
end

Think before deciding code is violating DRY. Ask yourself if the code is really hurting maintainability. As with most principles, DRY has a gray area - just be careful not to take it too far.

Related: Shane Harvie blogged about abusing DRY in a post on Moist Tests yesterday.