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])
    @account =[:account])
    @business =[:business])
    @main_address =[:main_address])
    @billing_address =[:billing_address])
    @delivery_address =[:delivery_address])
      @user.account = @account
      @account.customer = @business
      @account.main_address = @main_address
      @account.billing_address = @billing_address unless
      @account.delivery_address = @delivery_address unless
      @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
!                                           #is needed because the
!                                        #valid_answer? method
!                                       #deletes the captcha-image
!                                   #and resets the challenge
! unless @account.billing_address_same_as_main
! unless @account.delivery_address_same_as_main
          render :action => 'thanks_for_signup'
          @captcha = nil
          @captcha_error = true
      else               #still need to validate other models if one of them is invalid
        @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)
  def start_captcha
      session[:tc] = tc = => 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)

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])
  @user.account =[:account])
  @user.account.customer =[: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.

#in the controller you just do:

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

Re: cleaning up a controller action

many thanks for the quick and helpful responses!