HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

How might I make this code more DRY?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thismightmakemoredryhowcode

Problem

I have the following Ruby code in an RSpec file:

describe "order" do
    before do
      LIST_LENGTH ||= 10
      @skills = FactoryGirl.create_list(:skill, LIST_LENGTH)
      @developer = FactoryGirl.create(:user)
      @requests = FactoryGirl.create_list(:request, LIST_LENGTH)
      LIST_LENGTH.times { |i| @requests[i].skills = @skills[0..i] }
    end 

    describe "#order_by_interestingness_for" do
      it "orders by interestingness" do
        @developer.add_interested_skills(@skills)
        Request.all.shuffle.map {
          |r| r.interestingness_for(@developer)
        }.should_not be_in_order

        Request.order_by_interestingness_for(@developer).map {
          |r| r.interestingness_for(@developer)
        }.should be_in_order
      end 
    end 

    describe "#order_by_qualifiedness_for" do
      it "orders by interestingness" do
        @developer.add_proficient_skills(@skills)
        Request.all.shuffle.map {
          |r| r.qualifiedness_for(@developer)
        }.should_not be_in_order

        Request.order(:interestingness, :developer => @developer).map {
          |r| r.qualifiedness_for(@developer)
        }.should be_in_order
      end 
    end 
  end


Obviously, this is not very DRY. I don't really know how to make it more DRY, though, since the the unique parts are so deeply ingrained in the code. I'm guessing I have a deeper design problem. I'm not really sure, though. Any ideas?

Solution

Some suggestions:

  • There's no need for the negative assertion. You're testing Ruby more than you're testing your own code and in the end, we don't actually care. It only matters that our attempt to order the results behaves as expected. Assuming they're un-ordered otherwise (or at least order isn't guaranteed).



  • Favor RSpec's 'let' over instance variables and constants in your tests.



  • RSpec's 'should' syntax will be deprecated in the next release. Favor using the new 'expect' syntax instead.



  • Use string literals when you can.



  • Use a shared example to DRY up your tests a bit.



  • Modify the underlying implementation and the public API in your code for better symmetry between ordering by 'interestingness' and 'qualifiedness'. I suspect there's some code duplication in there given the current API that could also be eliminated.



Do all that, and you could end up with a test that looks something like this:

shared_examples_for 'a ordered request' do |order_type, dev|
  it "orders by #{order_type}" do
    result = Request.call(:order, { order_type, :developer => dev })
    result.map! { |r| r.send("#{order_type}_for", dev) }
    expect(result).to be_in_order
  end
end

describe Request do
  let(:list_length) { 10 }
  let(:skills) { FactoryGirl.create_list(:skill, list_length) }
  let(:developer) { FactoryGirl.create(:user) }
  let(:requests) { FactoryGirl.create_list(:request, list_length) }

  before { list_length.times { |i| requests[i].skills = skills[0..i] } }

  describe '#order' do
    context 'when ordering by interestingness'
      before { developer.add_interested_skills(skills) }
      it_behaves_like 'a ordered request', 'interestingness', developer
    do

    context 'when ordering by qualifiedness' do
      before { developer.add_proficient_skills(skills) }
      it_behaves_like 'a ordered request', 'qualifiedness', developer
    end
  end
end


I'd also recommend checking out this site which is a pretty good, but still growing style guide for writing behavior tests in RSpec.

Code Snippets

shared_examples_for 'a ordered request' do |order_type, dev|
  it "orders by #{order_type}" do
    result = Request.call(:order, { order_type, :developer => dev })
    result.map! { |r| r.send("#{order_type}_for", dev) }
    expect(result).to be_in_order
  end
end

describe Request do
  let(:list_length) { 10 }
  let(:skills) { FactoryGirl.create_list(:skill, list_length) }
  let(:developer) { FactoryGirl.create(:user) }
  let(:requests) { FactoryGirl.create_list(:request, list_length) }

  before { list_length.times { |i| requests[i].skills = skills[0..i] } }

  describe '#order' do
    context 'when ordering by interestingness'
      before { developer.add_interested_skills(skills) }
      it_behaves_like 'a ordered request', 'interestingness', developer
    do

    context 'when ordering by qualifiedness' do
      before { developer.add_proficient_skills(skills) }
      it_behaves_like 'a ordered request', 'qualifiedness', developer
    end
  end
end

Context

StackExchange Code Review Q#16127, answer score: 2

Revisions (0)

No revisions yet.