patternrubyMinor
How might I make this code more DRY?
Viewed 0 times
thismightmakemoredryhowcode
Problem
I have the following Ruby code in an RSpec file:
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?
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
endObviously, 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:
Do all that, and you could end up with a test that looks something like this:
I'd also recommend checking out this site which is a pretty good, but still growing style guide for writing behavior tests in RSpec.
- 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
endI'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
endContext
StackExchange Code Review Q#16127, answer score: 2
Revisions (0)
No revisions yet.