I recently morphed the complex multi-partial table-building system of my Ruby on Rails app, ExpenseLynx, into a more generic helper method. This is the story of how I arrived at the decision and test drove the result.
I’ve just been developing a hair over two years now, but one thing I’ve heard
consistently since the beginning is that helpers are undesirable. Dare I say
excessive use of helpers is even considered a
smell.
As a direct result, the very idea of adding to the generated *Helper
classes
in a Rails application rarely crosses my mind.
Receipt Tables
I’m displaying information about receipts in several places in ExpenseLynx. The information displayed with the receipts is context-sensitive, with the goal of enhancing the user experience by shedding information that isn’t relevant.
Creating Expense Reports
If we’re creating expense reports, for example, we already know the receipts we’re looking at are expensable and unexpensed; displaying this information would just be clutter. Showing whether or not these receipts are ready for a full export to CSV is helpful. Further, we need to be able to select any combination of receipts and build an expense report from those.
Viewing Receipts
For the vast majority of interaction, one doesn’t need to leave the main screen. Entering receipts sends an AJAX call to the server, which then returns a new table of the most recent receipts. There are no editing functions in this context as of now, and thus checkboxes in a row are unnecessary. This view does essentially serve as a confirmation of entered receipts, which is why the expensable status is useful.
Partials
Duplicated code is a bad thing. Always.
My first solution for this table appearing everywhere was to create a partial
called _receipt_table.slim
. To get rid of the iterating logic inherent in
creating tables, I took advantage of RoR’s facility for passing a collection of
receipts to another partial called _receipt.slim
. The framework delightfully
handles iteration.
As shown above, these tables aren’t completely identical. There are six columns displayed in each, but only three are common. We could easily envision a seven column scenario where expense status is displayed and checkboxes appear for mass edit-ability. Every different display requires some conditional logic. This is where the partial system I had fell down and became brittle.
Adding a Receipt Table to a View
In order to add a receipt table with specified columns, one would have to first
call the _receipt_table.slim
partial, horrifically specifying all the local
variables used in the template.
== render :partial => "receipts/receipt_table", \
:collection => @receipts, \
:locals => { :expensable => expensable, \
:expensed => expensed }
This code would predictably render the called partial with the appropriate table
headers. It then passes the collection and the previously mentioned local
variables through to the _receipt.slim
partial.
table#recent_receipts.summary_table
tr
th Date
th Store
th Total
- if expensable
th Expensable?
- if expensed
th Expensed?
== render :partial => "receipts/receipt", \
:collection => receipts, \
:locals => { :expensable => expensable, \
:expensed => expensed }
The _receipt.slim
partial is called once for every item in the collection,
where the passed in local variables are evaluated again to render the
appropriate table cells.
tr
td= receipt.purchase_date.to_date.to_s(:short)
td= link_to receipt.store.name, edit_store_path(receipt.store)
td.right= link_to number_to_currency(receipt.total), receipt
- if expensable
td.center= boolean_to_check receipt.expensable?
- if expensed
td.center= boolean_to_check receipt.expensed?
So, What’s the Problem?
Certainly the code above wasn’t going to win any elegance competitions. It did work though, and that was good enough at the moment. The biggest problem showed itself when I was adding the Export Ready? column. To add my column to the partials, I would have had to do several things:
- Add an
if
statement and new column name to_receipt_table.slim
- Add an
if
statement and new column name to_receipt.slim
- Change
_receipt_table.slim
to pass a new local variable into_receipt.slim
- Manually track down every instance of
_receipt_table.slim
and pass a new local variable with a value offalse
in
I found it so difficult to reuse these partials that I just ended up rewriting the table…which clearly duplicated code and gave me all the hints necessary that it was time to rework the partial system I had created. At a minimum, I want two things:
- open only one file to change column headers and cells
- not have to track down all usages when I modify the table
grid_for Helper
I had an idea of the syntax I wanted beforehand. Ideally, I’d like to be able to display a default grid easily:
== grid_for @receipts
Adding options should also be in a sentence-like, declarative way:
== grid_for @receipts, { :shows_expense_status => true }
In this style, we’re avoiding passing difficult-to-remember :locals
around,
which as we saw above are sort of duplicative in their own right. We can
abstract further than Expensable? and Expensed? since they always appear in
tandem.
To get there, I test drove my helper with rspec tests.
require 'spec_helper.rb'
describe :grid_for do
before do
@receipts = [Receipt.make!(:colins_unexpensed_tv_from_circuit_city)]
end
context :default do
subject { grid_for(@receipts) }
it { should include "Purchase Date" }
it { should include "Store" }
it { should include "Total" }
it { should include @receipts.first.purchase_date.to_s }
it { should include @receipts.first.store.name }
it { should include @receipts.first.total.to_s }
end
end
Which ultimately yielded this implementation of grid_for
def grid_for(receipts, options = {})
options.each { |opt| opt = false if opt.nil? }
render :partial => "receipts/table",
:locals => { :receipts => receipts,
:editable => options[:editable],
:shows_expense_status => options[:shows_expense_status],
:shows_export_status => options[:shows_export_status] }
end
The new helper is still calling a partial template, which effectively just
combined _receipt_table.slim
and _receipt.slim
. Opening this one file now
allows me to add to table headers and cells.
table#recent_receipts.summary_table
thead
tr
- if editable
th= check_box_tag "select_all"
th Purchase Date
th Store
th Total
- if shows_expense_status
th Expensable?
th Expensed?
- if shows_export_status
th Export Ready?
tbody
- receipts.each do |r|
tr
- if editable
td= check_box_tag "receipt_ids[]", r.id
td= r.purchase_date
td= link_to r.store_name, [:edit, r.store]
td.right= link_to money(r.total_money), [r]
- if shows_expense_status
td.center= boolean_to_check r.expensable?
td.center= boolean_to_check r.expensed?
- if shows_export_status
td.center.exportable= boolean_to_check r.exportable?
Retrospective
As was the goal with Pragmatic Refactor: JavaScript Confirmation Messages, we found something in a painful state and rearranged it to remove roughly half the pain.
Positives
- Our views can now deal with a higher level of abstraction that doesn’t require us to track down each usage with new additions to the table
- Our
grid_for
helper is easier to modify than the series of partials - Table headers and cells can be added in the same file
Things to Improve
- Adding a table still requires me to add the same
if
statement for table headers and cells - If I were to add a new conditional column in the table, I’d still need to add to the markup file and the helper file
Questions
- Can this be a gem? Didn’t find anything when briefly searching
Fun Fact The only thing currently evaluated in Export Ready? is whether or not the receipt’s store has an expense category.
Fun Files At the time of this posting:
application_helper.rb
application_helper_spec.rb
(see the rest of the tests!)_table.slim