It’s generally known that leaving any kind of logic in a Rails view is bad news, both for debugging and your own sanity. Rails views can be cumbersome to test and leave a lot to be desired when it comes to debugging.

I recently went through the process of refactoring a Rails view that included logic. The end result was an isolated PORO that was easily integrated with the controller/view.

Background

The app I’m currently working on is a greenfield app with vague specs, at best. I don’t mention this to fault anyone, but more to illustrate a point. Not all greenfield projects have well-defined specs.

In this particular case, the stakeholders were somewhat unsure of what the interface should look like. Together, we tossed around a number of ideas, ultimately leading to a few options. Only once an iteration of the UI was available, would we have a clear picture of whether it felt right.

The Problem

I implemented the first option in the most crude way I could think of. Unfortunately, that way involved putting logic in the view. I know, I know — I can hear it now, ”C’mon Brandon, everyone knows you shouldn’t do this!”. Here’s the thing — I knew it too.

Here’s what I ended up with:

# controller

def people
  @week_of = params[:week_of] || Checkin::Week.recent(current_company).first.beginning
  @people = current_user.reports
end

# view

<% @people.each do |person| %>
  <% checkin = person.checkin_for(@week_of) %>
  <tr>
    <td>
      <%= profile_picture person %>
      <%= person %>
    </td>

    <td>
      <%= checkin_status(checkin) %>
    </td>
  </tr>
<% end %>

For each iteration of @people, I looked up the check-in for that particular week from the model:

def checkin_for(week_of)
  Checkin.find_by(user_id: id, week_of: week_of)
end

This crosses concerns, blurs responsibility — all the things that bad Rails app are made of. But I was doing this knowing it would either be entirely ripped out (we’d change the UI altogether), or refactored to something better.

Note: I could’ve just saved myself one step and never made the method in the model. For some reason, that made me feel better about it at the time. *shrugs*

So I added a Github issue and went on my way…

The Solution

With a few minor tweaks, this implementation of the functionality and UI was adequate. So as time allowed, I jumped back in to untangling the mess I created.

The biggest variable in the display of a Checkin was the week (a date field corresponding to the beginning of that particular week). Once the date was known, I could look for a Checkin for each user in my visibility, see if it existed, and if not, return a stand-in object to represent a non-completed check-in.

I removed the model method:

def checkin_for(week_of)
  Checkin.find_by(user_id: id, week_of: week_of)
end

and the line in the view largely responsible for the mess:

<% checkin = person.checkin_for(@week_of) %>

I went back to the controller and initialized a new object that would allow me to iterate over a list of check-ins:

def people
  @week_of = params[:week_of] || Checkin::Week.recent(current_company).first.beginning
  @reports = Checkin::Reports.new(current_user, @week_of)
end

Let’s dig in to the new Checkin::Reports class…

class Checkin
  class Reports
    def initialize(user, week_of)
      @user, @week_of = user, week_of
    end

    def checkins
      reports.map { |person|
        Checkin.find_or_initialize_by(user_id: person.id, week_of: week_of)
      }
    end

    private

    attr_reader :user, :week_of

    def reports
      user.reports
    end
  end
end

I instantiate with the user and week, similar to what the checkin_for model method did above.

The only relatively interesting part of this class is that it’s using find_or_intialize_by. It turns out that the view didn’t care if the object was nil, or just a non-persisted Checkin object. All it did was interrogate certain attributes of the object, and guard against the argument being nil. From that perspective, we’ve improved our code even more because now instead of supplying nil to the helper, we are actually supplying it with a newly instantiated Checkin with some default attributes.

This means that our checkin_status helper, went from:

def checkin_status(checkin)
  if checkin && checkin.completed_at?
    fa_icon("check-square-o", class: "green") + " Submitted #{local_time checkin.completed_at, format: :short_month_day }".html_safe
  else
    fa_icon("warning", class: "yellow") + " Not completed"
  end
end

to:

def checkin_status(checkin)
  if checkin.completed_at?
    fa_icon("check-square-o", class: "green") + " Submitted #{local_time checkin.completed_at, format: :short_month_day }".html_safe
  else
    fa_icon("warning", class: "yellow") + " Not completed"
  end
end

It’s a subtle change (removing the check if checkin from the first conditional), but one that’s less susceptible to bizarre edge cases. And clearer — It’s reasonable to expect that by calling the first argument checkin, the variable should be a Checkin, and not sometimes nil.

Returning back to the view…using the new @reports variable, we no longer have to query during each iteration:

<% @reports.checkins.each do |checkin| %>
  <tr>
    <td>
      <%= profile_picture checkin.user %>
      <%= checkin.user %>
    </td>

    <td>
      <%= checkin_status(checkin) %>
    </td>
  </tr>
<% end %>

Tests pass, and we’re in a much better place than we were before.

Summary

I’ve seen a lot of people make notes for themselves to improve areas of their application and either never get the opportunity to go back and do so, or get so far removed from the mess, they forget about how bad it was in the first place. The approach I took above by making a note for myself worked because I knew I would go back to it. It’s possible this may not work for everyone.

If it takes more than just making a Github issue for your and your team, find what works. The important part is that the refactor takes place, in whatever way convenient for you.

The idea of not putting logic in a Rails view is well regarded as a best practice. Don’t think that just because I did it above, I’m advocating that it’s ok. To me, it’s only acceptable if you go back at a later time (soonish…) and clean it up.

Leaving little bits of bad practice sprinkled all of your app is heading of for a bad time. As Sandi Metz says, “go ahead, make a mess”. Just be sure to come back and clean it up.