Rado's blog

Rails Anti-Pattern: Setting View Variables In Before Actions

I'm seeing a lot of the following pattern in Rails projects:

class ProductsController < ApplicationController  
  before_action :assign_post, only: %i(show edit)

  def index
    @posts = Post.paginate(params[:page])
  end

  def show
  end

  def edit
  end

  private

  def assign_post
    @post = Post.find(params[:id])
  end
end  

Most people do this, because for them @post = Post.find(params[:id]) is duplication. Having assign_post is more DRY.

I don't buy the duplication argument. The definition for DRY is:

“Every piece of knowledge must have a single, unambiguous, authoritative representation within a system."

Unfortunately, most people confused it with - "I don't want to have the same sequence of characters more than once". David Chelimsky have an excellent talk on that subject.

But, my biggest issue with the approach is that it "hides" all variables passed to the view. Especailly if there are only or except passed to before_action. Then you have to become an interperter to figure out what gets into the view. I can recall several situations when during a code review I found code passing unwanted variables to a view.

I prefer to think the following, when I read a controller:

ok, show sends @post and @comments to the view

Instead of:

So, if show is called then assign_posts is called before it, which sets @posts, then show is not included in this other action filter, but is not excluded for this third one...

Also it is easier for a developer to spot, that too many variables are passed to the view if all of them are listed together.

Because of that, I prefer to use the following pattern:

class ProductsController < ApplicationController  
  def show
    @post = find_post
  end

  def edit
    @post = find_post
  end

  private

  def find_post
    Post.find(params[:id])
  end
end  

In this way the duplication is moved into finder methods. So if I have to add a scope like visible to Post.find, I just need to apply it in find_post.

Introducing MiniForm

My last blog post - Dealing with form objects, got me thinking about form objects. I also had a 12 hours flight to San Francisco, where I was meeting my teammates at Product Hunt.

During that flight, I went back and checked all of my custom form implementations. I gathered them togheter in a folder and start combining and extracting the common parts. By the end of the flight I almost had what now is MiniForm. (I had to change its name several times, since I'm really bad with names and most of the goodnames were taken).

MiniForm allows the following code:

 class RegistrationForm  
  include ActiveModel::Model

  attr_reader :user, :account

  attr_accessor :first_name, :last_name, :email, :name, :plan, :terms_of_service

  # user validation
  validates :first_name, presence: true
  validates :last_name, presence: true
  validates :email, presence: true, email: true

  # account validation
  validates :account_name, presence: true

  # form custom validation
  validates :plan, inclusion: {in AccountPlan::VALUES}
  validates :terms_of_service, acceptance: true

  # ensure uniqueness
  validate :ensure_unique_user_email
  validate :ensure_unique_account_name

  def initialize
    @user    = User.new
    @account = Account.new owner: @user
  end

  def update(attributes)
    attributes.each do |name, value|
      public_send "#{name}=", value
    end

    if valid? & user.update(user_attributes) && account.update(account_attributes)
      account.users << user
      AccountWasCreated.perform_later(account)
    else
      false
    end
  end

  private

  def ensure_unique_user_email
    errors.add :email, 'already taken' if User.where(email: email).any?
  end

  def ensure_unique_account_name
    errors.add :name, 'already taken' if Account.where(name: name).any?
  end

  def user_attributes
    {first_name: first_name, last_name: last_name, email: email}
  end

  def account_attributes
    {plan: plan, name: name}
  end
end  

To be written just as:

class RegistrationForm  
  include MiniForm::Model

  # `model` delegates attributes to models 
  # and copies the validations from them
  model :user, attributes: %i(first_name last_name email), save: true
  model :account, attributes: %i(name plan), save: true

  attributes :terms_of_service

  validates :plan, inclusion: {in AccountPlan::VALUES}
  validates :terms_of_service, acceptance: true

  def initialize
    @user    = User.new
    @account = Account.new owner: @user
  end

  # `update` calls `perform` if model is valid
  # and models are saved
  def perform
    account.users << user
    AccountWasCreated.perform_later(account)
  end
end  

Dealing with form objects

Lets say - we have the following nice clean model for our new application:

class Label < ActiveRecord::Base  
  belogns_to :user, required: true

  validates :name, presence: true, uniquness: true
end  

Several months later, we have the requirement to add quote and description to Label and those attributes should be mandatory. So we update Label:

class Label < ActiveRecord::Base  
  belogns_to :user, required: true

  validates :name, presence: true, uniquness: true

  validates :quote, :description, presence: true
 end

Now we have a big problem. All of our previous Label records are invalid. Since the new attributes didn't exist an hour ago. Also quote and description are not the kind of fields we add a decent default values for.

So we revert this change and start thinking for a better way to handle the situation.

When a little more thought is put into this - we find that we actually need this validation to be applied in two places - my/labels/create and my/labels/update pages.

One pattern I have seen used for dealing with this is the following:

class Label < ActiveRecord::Base  
  belogns_to :user, required: true

  validates :name, presence: true, uniquness: true

  validates :quote, presence: true, if: :validating_as_designer?
  validates :description, presence: true, if: :validating_as_designer?

  def update_as_designer(attributes = {})
    @validating_as_designer = true

    update_attributes(attributes).tap do
      @validating_as_designer = false
    end
  end

  private

  def validating_as_designer?
    @validating_as_designer
  end
end  

In a newish model, this approach doesn't look so bad.

But becomes unmaintainable really easy. The main issue here is that we adding form specific logic into a domain model. Now this model knows about a certain pages. In my experience such things tend to happen for the core modals of the application, which are bigger by nature.

Also I have seen situations where a new developer enters the project, see that update_as_designer is used and decided that "this is the way they do stuff here" and adds update_as_admin.

So what is a better way to deal with issue? I tend to favor the extraction of form objects:

class DesignerForm  
  include ActiveModel::Model

  attr_accessor :label, :user, :name, :quote, :description

  validates :user, presence: true
  validates :name, presence: true
  validates :quote, presence: true
  validates :description, presence: true

  def initialize(label = Label.new)
    @label = label
  end

  def persisted?
    @label.persisted?
  end

  def update(attributes)
    attributes.each do |name, value|
      public_send "#{name}=", value
    end

    if valid?
      @label.update attributes
    else
      false
    end
  end
end  

That sure looks too complicated! I can understand why people decide to use the "nice short method" instead of this big bulky object. But this is just our starting point.

Lets apply some minor modules, I use in almost every project I'm working on.

class DesignerForm  
  include ActiveModel::Model
  include ActiveModel::Attributes

  attr_reader :label

  delegate :persisted?, to: label

  # ActiveModel::Attributes method:
  # share attributes between form object and label
  attributes :user, :name, :quote, :description, delegate: :label

  # ValidValidator:
  # runs label validations and copies the errors to the form
  validates :label, valid: true

  # this is form specific validations we need
  validates :quote, presence: true
  validates :description, presence: true

  def initialize(label = Label.new)
    @label = label
  end

  def update(attributes)
    # ActiveModel::Attributes method
    self.attributes = attributes

    if valid?
      @label.save!
      true
    else
      false
    end
  end
end  

That is a lot better. Still not perfect. In my experience after having 2-3 form objects in the application - more and more common functionally is extracted. In the end most of the form objects become something like following:

class DesignerForm  
  include BaseForm

  model :label, attributes: %i(user name quote description) 

  validates :quote, presence: true
  validates :description, presence: true
end  

I don't start with BaseForm because every project has slightly different requirements for handling forms.

p.s. As expected - there a several gems which provide nice apis for form objects. I personally don't use any of them.

SearchObject 1.1

Today I release version 1.1 of my Search Object. It has two major new features.

Using instance method for straight dispatch

Suggested and developed by Genadi Samokovarov:

class ProductSearch  
  include SearchObject.module

  scope { Product.all }

  option :date, with: :parse_dates

  private

  def parse_dates(scope, value)
    # some "magic" method to parse dates
  end
end  
Classes mixed with SearchObject can be inheritted

I had this seeting in a branch for a long time. In one of the projects I worked on it would have been really usefull to have a base search object:

class BaseSearch  
  include SearchObject.module

  # ... options and configuration
end

# and used as 
class ProductSearch < BaseSearch  
  scope { Product }
end  

Its implementation actually might push me into a really interesting refactoring.

Other minor features
  • I added Rubocop for verifing the project
  • I started testing agains Ruby 2.2
  • I stopped testing agains Rubt 1.9

Preserving named arguments on inheritance

I don't use inheritance very often, but some times it is the best solution. Ruby is not much carrying when you overwrite a method. The method is just replaced by the new method. It doesn't care about arguments at all.

Since 2.0, Ruby support named arguments. I'm starting to use them more and more. But they are a bit tricky in terms of overwriting.

Lets say we have a class which creates user objects named UserBuilder:

class UserBuilder  
  def create(name: )
    puts "name: #{name}"
  end
end  

When I have to create a CustomerBuilder, which creates customers (user with address), I can write it like:

class CustomerBuilder < UserBuilder  
  def create(address:, name:)
    super name: name
    puts "address: #{address}"
  end
end  

This works, but creates a coupling between UserBuilder and CustomerBuilder. This coupling is a bit tricky to catch. Imagine the following scenarios:

  • name become optional in UserBuilder
  • name is renamed to full_name in UserBuilder
  • name is split into first_name and last_name in UserBuilder

In all of those cases change in UserBuilder triggers change in CustomerBuilder.

Such dependancies are toxic. Here is a better solution:

class CustomerBuilder < UserBuilder  
  def create(address:, **args)
    super **args
    puts "address: #{address}"
  end
end  

Now we can add, change, remove named arguments to UserBuilder#create and CustomerBuilder#create is not affected. Named arguments are just delegated down the change.