Rado's blog

Pulling back

Today, I was working on unknown for me, part of Product Hunt. I started with a dummy feature test similar to the following:

visit test_page

click_on_buttton  
enter_input_data

click_on_buttton  
enter_input_more_data

submit_form

check_expected_results  

Since this is the first of many tests, I added two hardcoded values to the view and the form object. I had a passing test.

So far all good :)

But when I ran the form object tests, they were broken, so I fixed them. Then I run the feature spec again, it was broken, so I fixed them. Then I ran the whole test suite and some tests were depending on the form object.

Almost a hour later, everything was green. But I had changed more than 20 files.

I could have made a commit and continue on my way. But I decided to pull back and do:

git reset --hard  

Before this I made a small TODO list for refactorings. So half a hour and couple of commits later - I had decoupled the form object from the rest of system and continue with my feature.

Why, I'm sharing this? In a lot of situations it is good to pull back and restart your work.

Custom routes in Rails

In the life of almost all Rails applications, I have ever worked on, comes the moment where custom routes are needed. Usually the custom routes are just convenience methods like these:

def post_path(post)  
  date_post_path post.date_slug, post.slug
end

def product_url  
  date_post_url post.date_slug, post.slug
end  

In this way you decouple the route generation from your objects. Also it is great if you are doing some sort of system route update and you want to keep the old routes around.

Usually, I create a module CustomPaths and put those methods there. But in recent years, I have noticed Rails.application.routes.url_helpers showing up in more and more places like - background jobs, presenters, serializers and so. This makes having a simple module not so easy.

At Product Hunt we needed some custom paths. So, me and Mike Coutermarsh created the following object:

# => routes.rb
module Routes  
  # Simple `extend self` won't work here,
  # due to the way Rails implement `url_helpers`
  class << self
    include Rails.application.routes.url_helpers
    include Routes::CustomPaths
  end

  include Rails.application.routes.url_helpers
  include Routes::CustomPaths

  def default_url_options
    Rails.application.routes.default_url_options
  end
end

# => routes/custom_paths.rb
module Routes  
  module CustomPaths
    # ... all your custom route methods
  end
end  

It behaves the same way as Rails.application.routes.url_helpers plus the custom routes.

# in can be called directly
Routes.root_path  
Routes.product_path(product)

# or included in other object
class ObjectWhoNeedsRoutes  
  include Routes
end

ObjectWhoNeedsRoutes.new.root_path  
ObjectWhoNeedsRoutes.new.product_path  

I added Routes to my toolbox with install instructions and tests.

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.