Naughty callback

I recently ran into what at first seemed like an odd problem with Rails while adding a simple before_create filter to one of my models. The code looked something like this:

# gummy_bear.rb
class GummyBear

  before_create :set_cheap_sweetener, :if => :need_to_cut_cost?

  private

  def set_cheap_sweetener
    self.high_fructose_corn_syrup = true
    self.sugar_cane = false
  end
end

After adding this before_create filter, my gummy bears would no longer save (ActiveRecord::RecordNotSaved exception). It didn’t hit me right away but I then realized that my callback always returned false. And given that the entire callback chain runs within the “save” transaction, that’s enough to make ActiveRecord choke and rollback the entire operation.

Here is what the ActiveRecord documentation says:

If a before_* callback returns false, all the later callbacks and the associated action are cancelled. If an after_* callback returns false, all the later callbacks are cancelled. Callbacks are generally run in the order they are defined, with the exception of callbacks defined as methods on the model, which are called last.

Lesson learned, moving on…

I like them lean

One of my personal objectives here at Fandor is to improve the overall code quality. This is a lengthy process which requires multiple refactoring iterations spread over time, especially considering that the product itself is constantly evolving. My strategy is simple: whenever I’m looking at an area of the product that I haven’t been exposed to yet, I spend some time analyzing the current design and see if there’s room for improvement. Typically if I’m having a hard time understanding the logic or I find the code too convoluted and not easy to read then it’s time for some refactoring. Here’s one example I recently came across.

The code smell

The code below, which has been slightly modified for clarity purposes, does something as trivial as registering a new user. Yet the implementation appears too verbose and too bloated, particularly given that it resides in a controller. The multiple model invocations and the multiple code paths are also good indications of a code smell.

# UsersController
def create
  @user = User.new(params[:user])
  render :action => "new" unless @user.valid?
  credit_card = CreditCard.new(params[:user][:credit_card])
  billing_address = { :street => params[:user][:billing_address][:address], :city => params[:user][:billing_address][:city], :state => params[:user][:billing_address][:state], :zipcode => params[:user][:billing_address][:zipcode] }
  if @user.valid_credit_card?(credit_card) && @user.valid_billing_address?(billing_address)
    begin
      if @user.register(@credit_card, @billing_address)
        flash[:notice] = "User successfully created"
        redirect_to @user
      else
        flash.now[:error] = "An error occurred while ..."
        render :action => "new"
      end
    rescue PaymentGatewayException => e
      flash.now[:error] = e.message
      render :action => "new"
    end
  else
    render :action => "new"
  end
end

The refactoring

What makes this implementation convoluted is that it manipulates three different models (user, credit_card and billing address) in an attempt to ultimately create one new user resource (after all we are in the create action of the UsersController). It’d be nice if we only had one model to manipulate.

The key insight is to recognize that, even though they are not persisted, the credit card and billing address do “belong” to the user. A user “has one” credit card and “has one” billing address even though there is no explicit has_one association defined between these models.

In other words the credit card and billing address should really be defined as virtual attributes of the user model. With this refactoring in place, the controller code becomes agnostic to the credit card and the billing address or said differently, the credit card and the billing address are now exclusively the concern of the user model. As a result of this separation of concern, the controller code can then focus on what it is meant to do, that is in this particular case, instantiate, validate and persist a new user, while the heavy lifting (e.g. instantiating and validating the credit card and billing address) is left to the user model.

I will spare you all the implementation details but to give you a sense of the refactoring that needed to be done, I’ve highlighted some of the code that was added to the user model.

# user.rb
[...]
attr_accessible :credit_card, :billing_address

validate :credit_card_attribute, :unless => :skip_billing_validations
validate :billing_address_attribute, :unless => :skip_billing_validations

[...]

def billing_address
  @billing_address ||= BillingAddress.new
end

def billing_address=(hash)
  @billing_address = BillingAddress.new(hash)
end

def credit_card
  @credit_card ||= CreditCard.new
end

def credit_card=(hash)
  @credit_card = CreditCard.new(hash)
end

private

def credit_card_attribute
  self.credit_card.valid?
end

def billing_address_attribute
  self.billing_address.valid?
end

[...]

As part of this refactoring, the credit card and billing address are now implemented as separate PORO classes (i.e. non-database backed models). I’m omitting their implementation since this is not the focus of this post. Notice however that they both respond to a valid? method, which will allow us to validate the user, credit card and billing address data all in one shot through a single invocation of user.valid? in the controller.

# users_controller.rb
def create
  @user = User.new(params[:user])
  if @user.valid?
    begin
      @user.register
    rescue PaymentGatewayException => e
      flash.now[:error] = e.message
      render :action => "new"
    else
      flash[:notice] = "User successfully created"
      redirect_to @user
    end
  else
    render :action => "new"
  end
end

The take away

I refactored this controller by applying the basic principles of separation of concerns. Another way to look at this is to think in terms of outside-in design. If you were to design this controller action from scratch starting from the user story, you’d probably come up with something to the extent of:

# Instantiate user using the input parameters
# Register user
# If registration is successful …
# Otherwise …

I personally find that even with more complex controllers, there’s generally no reason for your implementation to introduce a lot more complexity than that, even down the road as the product matures and new functionality is added. It sometimes takes a lot more efforts than in this particular simple example, especially when the existing design exhibits a tight coupling, but it’s usually well worth the effort.

Installing gherkin from a git repository with bundler

I just installed our Rails application on new hardware running Ubuntu 11.10. Everything went smoothly except for one gem: gherkin 1.0.30. (We’re on Rails 2 for now, so we’re using cucumber 0.8.0, which uses gherkin 1.) bundle install resulted in this error:

/usr/lib/ruby/vendor_ruby/1.8/rubygems/installer.rb:533:in `build_extensions': ERROR: Failed to build gem native extension. (Gem::Installer::ExtensionBuildError)
/usr/bin/ruby1.8 extconf.rb
gcc -I. -I. -I/usr/lib/ruby/1.8/x86_64-linux -I. -fPIC -fno-strict-aliasing -g -g -O2 -fPIC -O0 -Wall -Werror -c gherkin_lexer_ar.c
/Users/aslakhellesoy/scm/gherkin/tasks/../ragel/i18n/ar.c.rl: In function ‘CLexer_scan’:
/Users/aslakhellesoy/scm/gherkin/tasks/../ragel/i18n/ar.c.rl:198:29: error: the comparison will always evaluate as ‘true’ for the address of ‘raise_lexer_error’ will never be NULL [-Werror=address]
cc1: all warnings being treated as errors

Welcome to … Behind the Fan Door

By night we watch movies; by day we assemble the tubes through which you can watch them too, on your very own Internet. We are Fandor‘s engineering team.

We’ll use this blog to pass along some things we’ve learned while building our software. www.fandor.com is a Rails application; we can also sometimes be found Flashing. Soon we’ll be extending Fandor to new platforms with new engineering challenges. Hear it here first.

This is a work of engineering. Any resemblance to actual films, filthy or otherwise, is purely coincidental.