Prefer `if object.present?` over `if object`

So you’ve got a controller method

class DressesController < ApplicationController
  def show
    @dress = Dress.find_by(params.permit(:id))
  end
end

And a view

<div class="<%= dress ? 'dress&.status' : '' %>">Status Indicator</div>

Looks good! Works great! Now your app grows, and you have a lot of other views that use the dress. Long after your view has been forgotten, and there’s no clear owner because everyone owns it, but that’s cool because it’s regarded as stable, someone comes along and builds another view

<h1><%= dress.named? ? dress.name.gsub(' ','_').titleize : 'Unnamed' %></h1>

Ew gross, so much logic in the view? Gross! Let’s put that logic in a decorator!

class DressDecorator < Draper::Decorator
  delegate_all
  def title
    dress.named? ? dress.name.gsub(' ','_').titleize : 'Unnamed'
  end
end

delegate_all will forward all methods it's sent to whatever it's decorating.

class DressesController < ApplicationController
  def show
    @dress = DressDecorator.new(Dress.find_by(params.permit(:id)))
  end
end

And let's clean up our view.

<h1><%= dress.title %></h1>

But now @dress will always be there, even if it decorates nil, so our old code is broken, and will return

NoMethodError (undefined method `status' for #<DressDecorator:0x00007fbc2696d568>)

Because of this, it's always important to code your intention, rather than code to save keystrokes. Use dress.present?, because that's what you're checking, even though it's the same as dress right now.

<div class="<%= dress.present? ? 'dress&.status' : '' %>">Status Indicator</div>

In other words, watch out for this...

d = Dress.new(foo: 'bar')
!d.nil?
=> true
d.present?
=> true
d&.foo
=> 'bar'

dd = DressDecorator.new(d)
!dd.nil?
=> true
dd.present?
=> false
dd&.foo
Traceback (most recent call last):
NameError (undefined local variable or method `foo' for main:Object)

Post a Comment

Your email is kept private. Required fields are marked *