# Handling an e-commerce order form

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 :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

end

end

def populate
order.email = params[:email]

else
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


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.

• 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. – Mohamad Jul 29 '13 at 13:06

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

• 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 Jul 27 '13 at 17:47
• 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 :-) – tokland Jul 27 '13 at 18:55