threehv

I’ve recently been updating some old code – partly written by someone else, partly written by myself. At the time, I thought I had written this code really well; looking back on it now, it looks awful. Fair enough, I’ve learnt a lot – I want to look back on old code and shudder. But also, there is very poor test coverage on this app and the tests that there are are quite unwieldy due to an over-reliance on fixtures.

So I’ve been reworking them all using RSpec, my fork of RSpec-Rails and my Object Factory (which means I can avoid fixtures).

Most of the work involves writing a spec that mimics the current behaviour (by inspecting the code and trying to match all paths through it), then refactoring the code, using the spec to prove that I haven’t broken it.

But some points have some really horrible code (and lots of it) within the controllers. As you probably know, Skinny Controllers is the Rails way – your application logic belongs in your models (as they are your application) – the controller should just find or create the relevant model, ask it to do something and then render the results.

Because of this, I opted to just rewrite the actions in question.

To do this I started by writing a Cucumber feature describing things from the user’s point of view. Actually writing the steps that match the feature was a lot of work; because Cucumber is a full stack test you have to deal with all the dependencies that your individual action has (for example, are you logged in with the correct permissions with all associated objects created and in the right state?).

Then I wrote a controller spec. Controller specs in RSpec should use mock objects; you don’t really want to test the models, you just want to prove that the controller finds or creates the right model, asks it to do something and renders the correct output at the end.

So a typical spec looks something like this (note that this is not RESTful as it was an existing part of the application that I am about to change):

  it "should process an item" do
    @item = mock_model Item, :work_type => :buy_stuff, :quantity => 2

    on_getting :process_item, :id => '1' do
     Item.should_receive(:find).with('1').and_return(@item)
      @stuff = mock_model Stuff
      @item.should_receive(:process).and_return(@stuff)
    end

    assigns[:stuff].should == @stuff
    response.should be_success
    response.should render_template('admin/orders/process_item')
  end

This basically says:

• We have an item, of type “buy stuff” with a quantity
• When the process_item action is called, we expect that the controller will try to find the item with the given id
• Then we should call process on the item and it should give us some stuff
• The stuff should be stored in an instance variable, called stuff
• And a page should be successfully rendered using the process_item template

That’s a pretty succinct explanation of what the controller should do – I can’t think of many ways of making that skinnier. It also bears no resemblance to the actual implementation of the action – which currently looks something like this:

def process_work_item
    @item =Item.find(params[:id])
    case @item.product.class.to_s.underscore.to_sym
    when :buy_stuff
      @stuff = Stuff.build_item(@item)
      @stuff.setup_new
      render :action => :process_stuff
    when :update_stuff
      @item.stuff.prepare_for_update
      render :action => :update_stuff
    else
      render :status => 404, :text => "Item product type #{@item.product.class.to_s} unknown."
    end
rescue ActiveRecord::RecordNotFound
  render_not_found
end

Pretty complicated – and as new types of item are added we need to add more and more clauses to that case statement.

First things first – I’ve said that we should call “process” on the item class. So I add a pending spec to the Item specification – this is to remind me that I’ve got some work to implement later on.

  it "should process itself based upon its work type"

Then I rework the controller so that the controller spec passes.

    @item =Item.find(params[:id])
    @stuff = @item.process
    render :template => "admin/orders/process_item"

Pretty simple huh?

What we have done is shifted the logic from the controller into this new “process” method on the Item. We have made it so that the controller knows virtually nothing about the item – all it knows is how to find it and that it has a process method. All the implementation details are now hidden within the Item, out of the way of the outside world.

Through the use of mocking we can ignore actual implementations and concentrate on presenting ourselves as simply, and minimally, as possible to the outside world. This reduces coupling, increases flexibility and makes our code easier to read. Don’t you agree?

This entry was posted on Saturday, December 20th, 2008 at 6:14 pm and is filed under Designing Great Software, Ruby on Rails and Software Development, Writing Reliable, Bug-Free Code.