Now there is a controversial title.

I’m not trying to say that it’s dangerous to remove duplication in your code, merely that there are dangers that you might face when doing so. Duplication in code is a bad smell. It’s a reminder to us that we need to refactor it in some way in order to improve the design and make the duplication go away.

Sometimes, we get so caught up in the idea the removing the duplication is the only goal that we forget about the fact that this process is actually supposed to drive good object oriented design. Looking back at rails code that I’ve written over the last year, the place where I’m seeing this more than any other is in helpers.

Rails helper methods are places to extract logic from the view. They are tremendously handy, and a nice easy place to stick logic that appears in several different view templates, or contains so much conditional logic that it would make our rhtml templates unreadable. However, they are also just procedures. I find myself slipping into the trap of using procedural programming techniques to remove duplication here, rather than good design practices. Sometimes you can spot this because you find yourself starting to add a lot of arguments to you helper methods.

I’m going to share with you an example of some of my bad code. No programmer likes to do this, but I don’t feel too bad about it because I’ve just removed this method from a project, so I can at least say I did write this bad code, but now I’ve improved it.

Some Code in Need of Refactoring

    def editable_section_if(condition, name, resource_name = nil, form_url = nil)
      rollover_link_name = "rollover_link_#{name}" 
      display_div = "display_#{name}" 
      edit_div = "edit_#{name}" 
      partial_prefix = resource_name.nil? ? '' : "#{resource_name}/"       result = ""       if condition
        result << "<div class=\"editable_text\" onmouseover=\"Element.show('#{rollover_link_name}')\" onmouseout=\"Element.hide('#{rollover_link_name}')\" id=\"#{display_div}\">" 
        result << '<div class="rollover_link_bar">'
        result << link_to_function("Click to Edit", "Element.hide('#{display_div}'); Element.show('#{edit_div}')", {:class => "rollover_link", :id => rollover_link_name, :style => "display: none;"})
        result << '&nbsp;</div>'
        result << render(:partial => "#{partial_prefix}display_#{name}")
        result << '</div>'              result << "<div id=\"#{edit_div}\" style=\"display: none\">"         form_url = {:action => "update_#{name}", :id => params[:id]} if form_url.nil?
        form_url.merge!(:controller => resource_name) unless resource_name.nil?
        result << form_remote_tag(:url => form_url, :method => :put, :update => name, :loading => "Element.show('spinner_#{name}')", :loaded => "Element.hide('spinner_#{name}')", :html => {:class => 'inline'})        result << render(:partial => "#{partial_prefix}edit_#{name}")
        result << "<div class=\"editable_form_buttons\">" 
        result << submit_tag("Save")
        result << " or " 
        result << link_to_function("Cancel", "Element.hide('#{edit_div}'); Element.show('#{display_div}')")
        result << "</div>"         result << '</form>'
        result << '</div>'      
      else
        result << render(:partial => "#{partial_prefix}display_#{name}")
      end      result
    end

About the Code

This is a helper method to generate an inline form, which remains hidden in the page. The div displaying the data is loaded from a partial, as is the contents of the form, and edit and cancel buttons are used to toggle between them with javascript. The submit button uses ajax to submit the form. Also, it uses a condition to determine if the data is even editable, and if not, just displays the data, not the form.

This approach is a fairly standard thing when you realise that you want to do some inline editing that’s more than just one field, and so is a bit more complex that the built in rails inline editor helpers.

So what are the bad smells here?

First up is the really heavy string manipulation. I find myself doing this in helpers a fair bit. I probably started writing this in a rhtml template and then moved it into a helper when I needed to re-use it. The ERb of a rhtml template is more suitable for something which is so html heavy. All this string append operations, and bits of html everywhere make this method really hard to read. If I was missing a closing html tag, would I be able to easily tell? Do you find yourself writing helpers like this, or is it just me? Perhaps it should be a partial.

Secondly, the last two parameters were actually added latter. They sound important, but they have default values of nil. They are actually an attempt to try and make this system compatible with REST. They don’t entirely succeed, and this is an ugly function to try and make work in this situation.

Thirdly, this method is too long. Some programmers believe that methods of this length are just fine. If you are one such, please don’t ever bother to send me your resume.

I have already refactored this code, and I like the way it’s coming along, so keep an eye out for another blog post soon where I’ll suggest some better ways of doing this.