3
\$\begingroup\$

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
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Mohamad Jul 29 '13 at 13:06
1
\$\begingroup\$

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,...) 
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Mohamad Jul 27 '13 at 17:47
  • \$\begingroup\$ Can you comment why the cached objects are a bad idea here? And how would you get rid of them? \$\endgroup\$ – Mohamad Jul 27 '13 at 17:48
  • \$\begingroup\$ 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 :-) \$\endgroup\$ – tokland Jul 27 '13 at 18:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.