Handling an e-commerce order form

Question

I wrote this form object to handle an e-commerce order form. My areas of concern are the verbosity of the populate method, and the repetition of the validations. I omitted some address fields and validations for brevity.

class OrderForm
 include ActiveModel::Model
 def persisted?
 false
 end
 attr_accessor :params
 delegate :email, :bill_to_shipping_address, to: :order
 delegate :name, :street, :city, :state, :post_code, to: :shipping_address, prefix: :shipping
 delegate :name, :street, :city, :state, :post_code, to: :billing_address, prefix: :billing
 validates :email, length: { maximum: 60 }, email_format: true
 validates :shipping_name, :shipping_street, :shipping_city, presence: true
 validates :shipping_post_code, numericality: { only_integer: true }
 validates :billing_name, :billing_street, :shipping_city, presence: true, unless: -> { bill_to_shipping_address }
 validates :billing_post_code, numericality: { only_integer: true }, unless: -> { bill_to_shipping_address }
 def initialize(item, params = nil, customer = nil)
 @item, @params, @customer = item, params, customer
 end
 def submit
 populate
 if valid?
 order.save!
 true
 else
 false
 end
 end
 def order
 @order ||= @item.build_order do |order|
 order.customer = @customer if @customer
 end
 end
 private
 def shipping_address
 @shipping_address ||= order.build_shipping_address
 end
 def billing_address
 @billing_address ||= order.build_billing_address
 end
 def populate
 order.email = params[:email]
 order.bill_to_shipping_address = params[:bill_to_shipping_address]
 shipping_address.name = params[:shipping_name]
 shipping_address.street = params[:shipping_street]
 shipping_address.city = params[:shipping_city]
 shipping_address.state = params[:shipping_state]
 shipping_address.post_code = params[:shipping_post_code]
 if order.bill_to_shipping_address?
 billing_address.name = params[:shipping_name]
 billing_address.street = params[:shipping_street]
 billing_address.city = params[:shipping_city]
 billing_address.state = params[:shipping_state]
 billing_address.post_code = params[:shipping_post_code]
 else
 billing_address.name = params[:billing_name]
 billing_address.street = params[:billing_street]
 billing_address.city = params[:billing_city]
 billing_address.state = params[:billing_state]
 billing_address.post_code = params[:billing_post_code]
 end
 end
end

The Order object has billing_address and one shipping_address objects. They both inherit from an Address.

Here's the controller:

 def new
 @order_form = OrderForm.new(@item)
 end
 def create
 @order_form = OrderForm.new(@item, params[:order], current_user)
 if @order_form.submit
 # process payment
 else
 render 'new'
 end
 end

Answer 1

It seems to me Address is a value object, your billing and shipping address are just usages of this address. You are receiving either one or two addresses from the request. If you get one, you use the same address for billing as shipping.

I remember fighting nested_attributes a long time ago, but accepts_nested_attributes_for looks much easier.

7 Patterns to Refactor Fat ActiveRecord Models has a bunch of good advice on cleaning up rails code along with guidelines as to when to use each technique.

I haven't used Reform, but RailsCasts.com mentioned it when talking on the subject. It provides a DSL for form objects, so you can untie your model from your forms. I'm not sure you I feel about this, I worry it will mean duplicating validation logic. I would use something like this if I was attempting to build a strong domain model and didn't want the presentation logic leaking into the models. Whenever I get the need for a strong domain model (DDD) in Rails, I need to build the logic outside Rails (in a gem) and then integrate that in, so I end up with a Rails app doing what Rails does best, database backed web application.

• Mohamad: There's definitely a duplication of validation logic between the Address model and form object. I could not find a way to overcome the issues I mentioned in accepts_nested_attributes_for, and in the end it proved much harder to implement in a clean way. While this introduces some duplication in validation, it's far cleaner. The primary issue with accepts_nested is the need to create/validate billing address conditionally. That messes it up entirely. I would certainly be interested too see how you would solve this with nested attrs.

Answer 2

I was about to propose some changes to your code to reduce the line-count (and get rid of those @cached objects, bad idea IMHO), but I think it's the big picture what it's failing here. Some notes:

• You are creating an OrderForm abstraction when in fact Rails gives all the infrastructure you need to write this in Order in a simple and very concise way.

• All those validations belong to each model, validating them from an outside class is not a good practice.

• Use accepts_nested_attributes_for (in models) + fields_for (in views) so the associations are automatically (and transparently) handled by Rails.

[EDIT] Regarding the non-cached approach, a glimpse of what I'd write:

order = @item.order || build_order
order.customer = @customer
shipping_address = order.shipping_address || order.build_shipping_address
shipping_address.attributes = params.slice(:shipping_name, :shipping_street,...) 

• Mohamad: Interesting comments. My experience tells me otherwise. I found accepts_nested_attributes_for lacking here. It presented problems instead of solutions. For example. If bill_to_shipping_addess is checked, the user will still see errors for both addresses. You have to hack the error messages and remove them unless the user wants different addresses. You can use reject_if to overcome this. But if there is was a validation error on the shipping address, the fields for billing address will disappear. There are other problems too. I spent two days to make it work to no avail.
• Mohamad: Can you comment why the cached objects are a bad idea here? And how would you get rid of them?
• tokland: I see, you've already fought with accepts_nested_attributes. Indeed, it may be hard to make it sometimes. About not liking cached objects: I generally don't like cached objects in models at all, too indirect. I prefer more explicit approaches. You want a shipping_address? well, create one, use it and return it or whatever, no need for a method object that creates one or returns the one already existing behind your back. I see no benefits with this indirect, side-effected approach. But I prefer functional programming (and referential transparency) so you can freely say I am biased :-)