How do I refactor this object to lessen dependency on callbacks?

Question

I have an Order object that belongs_to a BillingAddress and a ShippingAddress. I want to present my user with only ShippingAddress fields and a checked checkbox indicating that the billing address matches the shipping address. If the user unchecks the box, the BillingAddress fields appear.

My implementation feels clunky and my Order object has a lot of callbacks.

class Order < ActiveRecord::Base
  attr_accessor :bill_to_shipping_address

  belongs_to :billing_address,  class_name: 'Address'
  belongs_to :shipping_address, class_name: 'Address'

  accepts_nested_attributes_for :billing_address, :shipping_address

  after_initialize  :set_billing_to_shipping_address
  before_validation :set_billing_address
  after_validation  :clear_billing_address_errors

  # Init the object with option checked
  def set_billing_to_shipping_address
    self.bill_to_shipping_address ||= '1'
  end

  # Copy shipping address attrs to billing address
  def set_billing_address
    self.billing_address = self.shipping_address if bill_to_shipping_address?
  end

  def bill_to_shipping_address?
    bill_to_shipping_address == '1'
  end

  # If shipping address matches billing, we copy the attrs, and thus duplicate errors too.
  # We only need to show the user one set of errors if addresses are the same, so remove them for billing address.
  def clear_billing_address_errors
    if bill_to_shipping_address?
      self.errors.messages.each { |k,v| self.errors.messages.delete(k) if k.to_s.split('.')[0] == 'billing_address' }
    end
  end
end

I have four methods along with three registered callbacks to satisfy this need. I'm also hacking around the error messages. I have no logic in the controller and the form is relatively simple.

= form_for @order do |f|
    # ...
    = f.label :bill_to_shipping_address, class: 'checkbox' do
      #{f.check_box :bill_to_shipping_address} Use my shipping address as my billing address.

Questions:

1. How can I improve my implementation?
2. Would switching the relationships help?--Order has_one :billing_address and has_one :shipping_address instead of belongs_to. Nested forms will feel more natural; in that, a parent creates children, not the other way around.

I'm reading a fair bit of refactoring books, but I can never map their examples to my own object design. I'm not that experienced I guess. I'm using Rails 4.

Answer 1

At first I don't think maintaining an attribute bill_to_shipping_address is necessary. In this case you need to keep three attributes: "shipping_address", "billing_address", and "billing_to_shipping_address". That's redundant.

In my opinion shipping address is shipping address and billing address is billing address. You always ship to shipping address and bill to billing address.

For the modelling, I assume there will be a shipping_address_id and billing_address_id in Order, all refer to a certain address id in Address.

In the view, you can handle it like this

• if no JS used. You provide another set of address fields, and told user to only fill it if they want to ship to an address different from billing address.

• If JS used, you can have a checkbox "Shipping to billing address" which is already checked. Once user uncheck it, you insert the new set of address field. Note: The checkbox itself makes no sense as a parameter and won't be considered in controller

• For a better user experience, you can add a #preview method which let the user finally confirm all input. Optional.

In controller#create,

• If shipping address fields have valid value, that means the user want a different shipping address. No need to compare it, just save the value into Address and the id in shipping_address_id.

• If shipping address fields are blank, that's fine, just copy billing_address_id into shipping_address_id.

Hope the above could give you some inspiration.

Answer 2

If they checked 'billing address same as shipping address', you shouldn't even try to validate or save a separate billing address, and therefore it shouldn't have any validation errors.

Since the form submit is creating multiple models, I'd suggest a separate OrderBuilder service object (there might be a better name) that you call from the controller. That way your order model doesn't need to be so concerned with clearing address errors. Your order builder can take care of creating the order and any address records, or copying over shipping address to billing address fields.

Also, your 'bill_to_shipping' should definitely be a boolean in the database. If you need to do any converting of the params to boolean, do it at the time you're saving the record, not every time you get it from the database.