Prefer locals over ivars

[frank-west-iii]

In controllers we should prefer passing locals explicitly rather than using ivars.

e.g. https://github.com/CenCalRuby/freshfoodconnect/blob/master/app/controllers/donors_controller.rb#L3

# Before
def show
  @donor = find_donor
end

# After
def show
  render :show, locals: { donor: find_donor }
end

[MarceloAlves]

Since specifying :show isn't necessary is it only done to be explicit?

[frank-west-iii]

@MarceloAlves, good question. I have got in the habit of this for the other cases and I keep this pattern so I am consistent, but I think that should be a separate issue.

We can solve the same problem here with simpler code:

# Before
def show
  @donor = find_donor
end

# After
def show
  render locals: { donor: find_donor }
end

[josephbridgwaterrowe]

@frank-west-iii, is there a reason you wouldn't also change the find_donor or is that highly subjective?

I've also included the explicit :show as I prefer being explicit and I don't think it hurts any (it's a little less magic in my opinion).

I also added a memoized backing value (yes, its an ivar, but prefixed with an understand to show the intent thats its "private"). Is this superfluous since ActiveRecord will use a cached result?

I think that the naming of existing_donor provides more context than find_donor. I may ask, find for what?, which could also be turned to ask which existing donor. What do you think about naming this to shown_donor? That being said, I do think existing_donor compliments new_donor when you have create and new actions.

However, I also feel like I may be debating the colour of the bike shed though.

For example:

class DonorsController < ApplicationController
  def show
    render :show, locals: { donor: existing_donor }
  end

  private

  def existing_donor
    @_existing_donor ||= User.donors.find(params[:id])
  end
end

[frank-west-iii]

TBH, I would just do this

class DonorsController < ApplicationController
  def show
    render :show, locals: { donor: donor }
  end

  private

  def donor
    @_donor ||= User.donors.find(params[:id])
  end
end

[frank-west-iii]

I realized that I didn't even explain why I prefer locals over ivars which is extremely unhelpful.

Take the following contrived example:

class SchedulesContoller < ApplicationController
  def show
    @user = User.find(params[:id])
  end
end

# app/views/schedules/show.html.erb
<div>
  Email:  <%= @user.email %>
</div>

<div>
  Name: <%= formatted_name %>
</div>

<%= render partial: "posts" %>

module SchedulesHelper
  def formatted_name
    "#{@user.first_name} #{@user.last_name}"
  end
end

# app/views/shared/_posts.html.erb

<% @user.posts.each do |post| %>
   # Render something
<% end %>

What happens when I do something like this:

class SchedulesContoller < ApplicationController
  def show
    @profile = Profile.new(User.find(params[:id])
  end
end

How many places do I have to fix this now?

So here we go we make the changes.

# app/views/schedules/show.html.erb
<div>
  Email:  <%= @profile.email %>
</div>

<div>
  Name: <%= formatted_name %>
</div>

<%= render partial: "posts" %>

module ApplicationHelper
  def formatted_name
    "#{@profile.first_name} #{@profile.last_name}"
  end
end

# app/views/shared/_posts.html.erb

<% @profile.posts.each do |post| %>
   # Render something
<% end %>

There, not so bad... except this just happened.

NoMethodError in Users#show
Showing users/show.html.erb where line #15 raised:
undefined method `first_name' for nil:NilClass

So we look at that file and it looks something like this on line 15:

<%= formatted_name %>

So although this can be avoided with ivars by passing them into your partials and helpers, the use of locals makes this kind of mistake impossible (AFAIK, happy to be challenged here).

I hope that is a clear enough example as to why I choose this design.

[josephbridgwaterrowe]

The idea that someone in the world may be writing helpers like that gives me the willies.

[josephbridgwaterrowe]

@frank-west-iii also re your TBH example... I would not disagree, but I believe being more explicit when there is some CRUD going on can be helpful.