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.