Refactoring Logic From a Rails View
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.
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.
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:
For each iteration of
@people, I looked up the check-in for that particular week from the model:
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…
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:
and the line in the view largely responsible for the mess:
I went back to the controller and initialized a new object that would allow me to iterate over a list of check-ins:
Let’s dig in to the new
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:
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
Returning back to the view…using the new
@reports variable, we no longer have to query during each iteration:
Tests pass, and we’re in a much better place than we were before.
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.