Topic: cleaning up a controller action

I want to cleanup this controller action.
It parses a signup-form which consists of quite a few models.
It feels a little big&heavy so I would like to refactor this to the/some model(s), but I can't realy know where to start.

Any help/suggestions?
Thanks in advance

  def account
    @user = User.new(params[:user])
    @account = Account.new(params[:account])
    @business = Business.new(params[:business])
    @main_address = Address.new(params[:main_address])
    @billing_address = Address.new(params[:billing_address])
    @delivery_address = Address.new(params[:delivery_address])
    if request.post?
      @user.account = @account
      @account.customer = @business
      @account.main_address = @main_address
      @account.billing_address = @billing_address unless
                    @account.billing_address_same_as_main?
      @account.delivery_address = @delivery_address unless
                    @account.delivery_address_same_as_main?
      @captcha = params[:captcha]
      if @user.valid? and @account.valid? and
             @business.valid? and @main_address.valid? and
             (@account.billing_address_same_as_main? or @billing_address.valid?) and
             (@account.delivery_address_same_as_main? or @delivery_address.valid?)
        if session[:tc].valid_answer?(session[:c].id, @captcha) #this extra if clause
          @user.save!                                           #is needed because the
          @account.save!                                        #valid_answer? method
          @business.save!                                       #deletes the captcha-image
          @main_address.save!                                   #and resets the challenge
          @billing_address.save! unless @account.billing_address_same_as_main
          @delivery_address.save! unless @account.delivery_address_same_as_main
          render :action => 'thanks_for_signup'
        else
          start_captcha
          @captcha = nil
          @captcha_error = true
        end
      else               #still need to validate other models if one of them is invalid
        @account.valid?
        @business.valid?
        @main_address.valid?
        @billing_address.valid? unless @account.billing_address_same_as_main
        @delivery_address.valid? unless @account.delivery_address_same_as_main
        @captcha_file = "captcha/" + File.basename(session[:c].file)
      end
    else
      start_captcha
    end
  end
 
  private
 
  def start_captcha
      session[:tc] = tc = Turing::Challenge.new(:lifetime => 600,
                          :store => 'store',
                          :width => 200,
                          :height => 90,
                          :outdir => "#{RAILS_ROOT}/public/images/captcha/")
      session[:c]  = c  = tc.generate_challenge
      @captcha_file = "captcha/" + File.basename(c.file)
  end

Re: cleaning up a controller action

The main improvement recommended is to follow rich-domain-model paradigms by pushing responsibilities into the models most fit to handle them.

Here are some things that can be done:

1. instead of creating models in one step, and then linking them in another step, you can do something like:

  @user = User.new(params[:user])
  @user.account = Account.new(params[:account])
  @user.account.customer = Business.new(params[:business])
  ...

That way, you save yourself quite a few lines of code.

2. Move all captcha-related logic into a captcha class that handles all its responsibilities. This is in accordance with rich-domain-model paradigms recommended earlier, which hide a lot of the complexities away in the captcha model and keep the controller easy-to-maintain.

3. You could potentially add a validate method on the top-most model in the hierarchy (account) to navigate through all submodels (associations) and validate them for you. That way, you only need to call @account.validate? instead of having to call valid? on all submodels too.

Good luck. I recommend reading "Applying UML and Design Patterns" by Craig Larman as a good introductory guide to rich-domain-model paradigms.

Last edited by AndyMaleh (2007-08-10 10:14:45)

Re: cleaning up a controller action

Additionally to what AndMaleh said:

Move the whole creation of the addresses and the logic like main_same_as_deliery? completely into the model, and validate the addresses themselves in this model:

class Account
  has_one :billing_address
  has_one :shipping_address
  has_one :main_address

  #validation of associated addresses, do the same in the user model for the account assocation etc.
  validates_associated :billing_address, :shipping_address, :main_address

def add_addresses(main_add,ship_add,bill_add)
# create you associated address objects here, dependend if some are the same or not.
end
end

#in the controller you just do:
...
@account.add_addresses(params[:main_address],params[:sipping_address],param[:delivery_address])
...
@user.valid?

Last edited by Duplex (2007-08-10 10:28:11)

Re: cleaning up a controller action

many thanks for the quick and helpful responses!