Slaying Dragons

The following is a complete piece of code from a real Rails app that is currently in production.

class Product < ActiveRecord::Base

  ...

  def self.supplied_within_states(*states)
    # So you can pass either an array or a list of arguments to the method
    states = states.flatten(1)

    if states.include? 'all'
      scoped
    else
      # Here be dragons! - RS 2013-05-20
      # Ideally this query would be much simpler - something like:
      # scoped.joins(:product_variants => {:supplier_product_variants => :supplier})
      #   .merge(Supplier.in_states(states))
      # to join all the tables and then filter on the final supplier table... however!
      # between Rails 3.0 not supporting nested has_many through associations (which is where the
      # supplier table comes from) and `merge` behaving a bit strangely with said nested HMT
      # (support for this got added by a gem).. this is the final result
      # It takes a couple of queries (two here and several in the Supplier.in_states class
      # method) but it does the job
      # When this app is upgraded to Rails 3.1+ this can be revisited (as it supports nested HMT
      # natively and probably has a bunch of other fixes too)
      scoped.where(:id => joins(:product_variants => {:supplier_product_variants => :supplier})
        .where(:suppliers => {:id => Supplier.in_states(states).select(:id)}))
    end
  end

  ...

end

As the comment states, I wrote it, when the app was using Rails 3.0 - and there are many dragons present. It’s a horribly nested query that maps through a couple of associations and then does a second lookup, that also uses a couple of associations… it does a lot of queries to get a simple result - a list of all products in the system that can be supplied within certain states of Australia.

In July 2014, the app got upgraded to Rails 3.2 - but this comment (and the related code) never got addressed… until now.

Last week I was revisiting this code as part of some major refactoring work - restructuring the associations between certain models, and removing the SupplierProductVariant model entirely. The last task I did was grepping the codebase looking for all references to the model, and fixing them, and I came across the comment.

I can address this comment, I thought. I can fix this code, and slay these dragons.

So what does the method look like now, after the code has been fixed and cleaned?

def self.supplied_within_states(*states)
  # So you can pass either an array or a list of arguments to the method
  states.flatten!(1)

  if states.include? 'all'
    scoped
  else
    joins(:product_suppliers => :supplier).merge(Supplier.in_states(states))
  end
end

It’s still not perfect (for example, the Supplier.in_states(states) method does a lot of nasty things) but it felt like a quick win. The method is much more readable, and clearer about what it’s actually doing. I’m being a good boy scout - leaving the codebase cleaner than when I found it - and it’s a great feeling.

All of our codebases have dragons lurking inside, some more obvious than others. You don’t have to hunt them out, but if you find them on your travels and fix them, especially in legacy code like this, you’ll feel much much better about working with the code in the future.

← Home