How to handle trivial "duplication of code" smell in Rails/Ruby

Question

So, we all strive to reduce duplication (DRY) and other smells, and keep our code as nice and clean as possible. For Ruby code, there are a lot of instruments to detect smells, for example the quite nice Caliber service.

However, it seems that I have a different definition of code duplication than the tools. I think this might be connected to the Ruby way of doing things, where you almost never access a variable directly, but through a method call instead. Consider this snippet from a Rails controller:

def update_site_settings
  SiteSettings.site_name = params[:site_name]
  SiteSettings.site_theme = params[:site_theme]
  expire_fragment('layout_header')
  flash[:notice] = t(:Site_settings_updated)
  redirect_to :controller => 'application', :action => 'edit_site_settings'
end

This is flagged with a code duplication warning, because of the two calls to the "params" method. So my question is, would it really be an improvement to assign the params to a local variable? I consider the way this is written to be the most clear and concise way to do it, and the fact that params is a method and not a variable is simple "the cost of doing business" in Ruby.

Am I seeing this the wrong way?

EDIT: In this case, a prettier way might be to do a SiteSettings.update_attributes(params) style update. Consider, if you will, the same problem in another snippet:

def update
  @mailing_list = MailingList.find(params[:id])

  if @mailing_list.update_attributes(params[:mailing_list])
    flash[:notice] = t:Mailing_list_updated
    redirect_to(mailing_lists_path)
    ...

Answer 1

One thing to remember about the concepts of DRY and code smells are that they are guidelines. They are there to help you think about how to organize and simplify your code in more of a forest-for-the-trees sense. While it is good to always keep these concepts in mind, nitpicking code repetition on a level this small will often end up introducing unnecessary complexity or obscurity into your code. The understandability of your code should be important too, and like you say, sometimes the code that is the most clear and concise is not the same as the code where every last trace of repetition is removed.

Answer 2

I suppose you could also declare a minor instance of the Feature Envy code smell, although it really is somewhat trivial.

Perhaps a class method on MailingList could be introduced, so the controller method becomes something like

def update
  if @mailing_list = MailingList.update_attributes_by_id(params)
    ...

class MailingList
  def self.update_attributes_by_id(params)
     id = params.delete(:id)
     find(id).update_attributes(params)
     ...

(not tested, so handle with care)

Would I bother in real life? Probably not, for one thing the two-part find/update thing is so common that people understand it immediately - someone coming to code like the above is going to have to stop and think a little, even if you have a perfectly expressive name.

These analysers (I run Kevin Rutherford's reek on my own code) are great, but they don't understand context so they're seldom going to offer perfect information. They're useful for identifying areas that may benefit from attention but there will be plenty of false positives and they'll also miss things, so they need to be used with awareness.