I took a few months leave of absence from Golf Trac. Well, the other evening I decided to go through the entire application and clean out the garbage (I had a feeling there’d be some). I forgot how surprisingly complex it was to model a golf scorecard with a normalized database and a single web form. But that’s not the point. The point is I love blocks in Ruby, particularly block helpers.
One of the things I came across was this _handicaps_and_pars.rhtml partial:
<tbody id="holes-<%= side %>"> <tr> <td>Holes</td> <%= hole_count_for(side) -%> </tr> <tr> <td>Men's Handicap</td> <%= render :partial => 'handicap', :collection => @course.holes, :locals => { :mens => true, :side => side } -%> </tr> <tr> <td>Men's Par</td> <%= render :partial => 'hole', :collection => @course.holes, :locals => { :mens => true, :side => side } -%> </tr> <tr> <td>Ladie's Handicap</td> <%= render :partial => 'handicap', :collection => @course.holes, :locals => { :mens => false, :side => side } -%> </tr> <tr> <td>Ladie's Par</td> <%= render :partial => 'hole', :collection => @course.holes, :locals => { :mens => false, :side => side } -%> </tr> </tbody>
It’s actually not that bad because it reveals what’s going on quite nicely. But there are just too many commonalities to leave it alone (the tr’s, td’s, :collection, :side, etc), so I added this little helper to assist with the problem:
def handicaps_and_pars(&block) titles = ["Men's Handicap", "Men's Par", "Ladies' Handicap", "Ladies' Par"] 4.times do |i| yield titles[i], i % 2 == 0 ? 'handicap' : 'hole', i < 2 ? true : false end end
And now the partial looks a little cleaner:
<tbody id="holes-<%= side %>"> <tr> <td>Holes</td> <%= hole_count_for(side) -%> </tr> <% handicaps_and_pars do |title, partial, is_mens| %> <tr> <td><%= title -%></td> <%= render :partial => partial, :collection => @course.holes, :locals => { :mens => is_mens, :side => side } -%> </tr> <% end -%> </tbody>
I could probably make the helper a bit more readable…
# inside the 4.times block title, partial, is_mens = titles[i], i % 2 == 0 ? 'handicap' : 'hole', i < 2 ? true : false yield title, partial, is_mens
...but I doubt that I will, considering I’m the only one working on the code.
I suppose another thing that would make the view more appealing would be to use Markaby (or something similar)... those <%= -%> tags are beginning to annoy me.
Comments
Does this mean it’s almost time for an ALPHA-BETA-PRE-PREVIEW launch…? :-)
Pretty please with a cherry or two on the top…
The old way was far more readable and maintainable, even if it did repeat itself.
That block of code is not only unreadable, it’s dangerous. If a maintenance programmer later comes through and wants to add a new form field, they will potentially see that titles array and cheerfully add it there, not realizing that the even/oddness and relation to the number 2 of the index is important.
Always plan for the future, even if not necessary at the moment. It makes for better code. Someday, when you come back to this code in a few years you’ll thank me.
Point taken.
I’m still figuring out when to stop breaking things down. I’ve gotten to the point where common, repeating code throws a flag, but in cases like this perhaps I should leave it be. Maybe it’s best to just repeat those four table row’s, even though I’m only toggling one or two variables.
Of course, had this been an application at work or one that had a team involved, I’d definitely be reluctant to write out a helper like that. But still… point taken.