Controller Duplication - Code Climate

Ruby on Rails
1

I love code climate. It’s great being able to write code and then immediately receive feedback on it’s quality.

One thing that is puzzling me is with some reported duplication. There is obviously a lot of duplication in our controllers, for example this update action in our ContractsController:

class ContractsController < ApplicationController
...
  def update
    @contract = Contract.find(params[:id])

    if @contract.update_attributes(contract_params)
      redirect_to :back
    else
      respond_to do |format|
        format.js
      end
    end
  end
...
end

Now I suppose we could dry up so we can use it in other controllers by moving the action into application controller:

class ApplicationController < ActionController::Base
...
  def update
    if @object.update_attributes(@object_params)
      redirect_to :back
    else
      respond_to do |format|
        format.js
      end
    end
  end
...
end

class ContractsController < ApplicationController
...
  def update
    @object = @contract = Contract.find(params[:id])
    @object_params = contract_params
    super
  end
...
end

But is this really a good idea?

2

I would not put the entire action in application controller, no. There are gems that do this type of thing for you - see inherited_resources – but I tend to dislike using them application wide. I’ve had some success using them in highly repetitive admin sections, but even then the barrier to understanding what the heck is going on is pretty high for newcomers to the code.

The particular bit of code you’re talking about can probably be cleaned up by using the class level respond_to macro copupled with respond_with in the action. This will use the default rails responder which is pretty smart about how to respond, but doesn’t always work exactly how you want it to work. You can create a custom responder that works exactly as you want.

I find custom responders to be a little less surprising than inherited resources. For more:
http://blog.plataformatec.com.br/2009/08/embracing-rest-with-mind-body-and-soul/
http://api.rubyonrails.org/classes/ActionController/Responder.html

3

Thanks @derekprior, I got the impression it was one of those “acceptable” duplications for clarities sake.

Just so I’m clear, I could rewrite this like so:

class ContractsController < ApplicationController
  respond_to :html, :js
  ...
  def update
    @contract = Contract.find(params[:id])
    @contract.update_attribute(contract_attributes)
    respond_with(@contract)
  end
  ...
end

Now this would load the update.js.erb template for a js request. Would I write in this template the logic for handling what happens if the record updates successfully or not? i.e.

<% if @contract.errors.present? %>
  $('#modalPopup .front .modal-body').html('<%= j render("form") %>').scrollTop(0);
<% else %>
  # something to load the new show page?
<% end %>
4

For HTML, the default, built-in responder behaves like this (this is simplified)

def to_html
    if get?
      render
    elsif has_errors?
      render :action => (post? ? :new : :edit)
    else
      redirect_to resource
    end
  end

So, if the request is a get it will simply render the template for the current action. If it is a post and has errors, the new action will be rendered. If it’s a put, patch or delete, then the edit action is rendered. If there were no errors and it was something other than a get then you are redirected to the show page.

For JS responses, I believe the default responder will either render the object as json or render the errors as json as appopriate. If you want to render a JS template then you would need to do something like:

respond_with(@contract) do |format|
    format.js { render 'contract/update' }
 end

Or, if this is the behavior you want everywhere, you can plug in a custom responder that doe this by default. I’ve never done this exactly, but in the past I have used customized responders to do things like redirect to an index page rather than the show page when a resource is created or updated successfully.

Responders are powerful, but unfortunately not super well understood by most people (myself included). I often reconsult Jose’s blog post (that I linked above). Here’s a couple more resources:

http://archives.ryandaigle.com/articles/2009/8/6/what-s-new-in-edge-rails-cleaner-restful-controllers-w-respond_with

5

Great stuff, thanks.

6

I’m a huge CodeClimate fan, and I use it for most of my client projects. However, I have found that trying to clean up this duplication may get you a better Reek/CodeClimate score, but it is at the price of readability.

I found it was difficult to instill as a habit, but learning to rely on static analysis tools but not to always change what they point to as smells is worth the will power it requires.

1 Like