I’m an OO bigot, plain & simple. One of my soapboxes/sacred-cows/hot-buttons/whatever is encapsulation… or more to the point, the lack of it.
I was writing some code recently and thought it might make a nice example.
I was just starting with a rough idea: I have a Form object that has FormPages. Pages can be moved around, added, deleted, etc. But they need to be ordered… pages of a form get filled out in some logical sequence.
So here’s my initial spec:
context "A form" do
setup do
@form = Form.new
@form.form_pages << FormPage.new(:name => 'one', :number => 3)
@form.form_pages << FormPage.new(:name => 'three', :number => 2)
@form.form_pages << FormPage.new(:name => 'two', :number => 1)
end
specify "should be able to order its pages" do
@form.ordered_pages.collect {|page| page.number}.should_eql [1, 2, 3]
end
end
Some code to make this work:
class Form < ActiveRecord::Base
has_many :form_pages
def ordered_pages
form_pages.sort
end
end
class FormPage < ActiveRecord::Base
belongs_to :form
has_many :form_fields
def <=>(other_page)
number <=> other_page.number
end
end
And that works just fine. BUT I’m totally violating any semblance of encapsulation. Look at FormPage for starters. We’re reaching into other_page and pulling out its number. Not so bad maybe.. it’s an instance of FormPage too. But it’s still breaking encapsulation. It can be done cleaner with a bit of double dispatch fun. Try this:
class FormPage < ActiveRecord::Base
belongs_to :form
has_many :form_fields
def <=>(other_page)
other_page.compare_number_to(number)
end
def compare_number_to(other_page_number)
other_page_number <=> number
end
end
That still works, and things are kept nicely encapsulated.
Now let’s look at the spec:
specify "should be able to order its pages" do
@form.ordered_pages.collect {|page| page.number}.should_eql [1, 2, 3]
end
That collect is leaving a bloody trail in its wake as it hacks its way through the pages, ripping out numbers like the organ collectors in Monty Python’s “The Meaning of Life”. What are the alternatives? Well.. to support sorting we just wrote FormPage#compare_number_to(other_page_number). Let’s see if we can’t use that:
specify "should be able to order its pages" do
@form.ordered_pages.each_with_index do |page, index|
page.compare_number_to(index + 1).should_eql 0
end
end
That does it. Notice that this works because we had that method to use, and the FormPages in the fixture were number sequentially.. we could clean it up even more by numbering the pages starting with 0 (which is much more ruby-esque anyway):
context "A form" do
setup do
@form = Form.new
@form.form_pages << FormPage.new(:name => 'one', :number => 2)
@form.form_pages << FormPage.new(:name => 'three', :number => 1)
@form.form_pages << FormPage.new(:name => 'two', :number => 0)
end
specify "should be able to order its pages" do
@form.ordered_pages.each_with_index do |page, index|
page.compare_number_to(index).should_eql 0
end
end
end
Nice. We can go further… I don’t really care for the exposure of the array of pages. I’d feel better with an addPage method:
class Form < ActiveRecord::Base
has_many :form_pages
def add_page(aPage)
form_pages << aPage
end
def ordered_pages
form_pages.sort
end
end
That lets us have our context setup be like this:
setup do
@form = Form.new
@form.add_page(FormPage.new(:name => 'one', :number => 2))
@form.add_page(FormPage.new(:name => 'three', :number => 1))
@form.add_page(FormPage.new(:name => 'two', :number => 0))
end
That’s much nicer from a pure OO perspective. “But,” you say, “that’s not overly Rails-like!” So… it IS very OO. It’s up to you where you draw the line. But remember… just because we can ignore encapsulation doesn’t mean we should. Doing so couples our code to the schema, and the goal of OO is to manage and minimize the coupling in our code. For a one-off, throw-away, 15 minute hack.. who cares.. but a mission critical application needs more thought & care. And it’s amazing how many one-off, throw-away, 15 minute hacks turn into mission critical apps.
David Chelimsky suggested making one more step and removing the reliance on the comparison method. After all, that is an implementation detail and relying on it from the spec makes it hard to change… just like any dependancy. So:
context "A form" do
setup do
@form = Form.new
@pages_in_order = [
FormPage.new(:id => 10, :name => 'two', :number => 0),
FormPage.new(:id => 5, :name => 'one', :number => 1),
FormPage.new(:id => 13, :name => 'zero', :number => 2)
]
@form.add_page(@pages_in_order[2])
@form.add_page(@pages_in_order[1])
@form.add_page(@pages_in_order[0])
end
specify "should be able to order its pages" do
@form.ordered_pages.each_with_index do |page, index|
page.should_equal @pages_in_order[index]
end
end
end
This final version is much better. I’ll blame my not seeing it on being sick at the time.
