From Good to Great

The third great conference talk I want to share is Ben Orenstein’s Aloha Ruby Conf 2012 talk called Refactoring from Good to Great.

Ben shows some common refactoring patterns by doing live coding on stage. He not only covers what he’s doing, but he also talks a lot about the why. I learned a tremendous amount from this talk.

Here are some of the topics he covers:

  • Extracting temporary variables to query
  • Tell don’t ask
  • Reducing parameter coupling by extracting data clumps into an object
  • Feature envy

And a few more.

My favourite quotes from the talk are:

I’m starting to think, these days, that methods longer than a line are a code smell.

Most intermediate object oriented programmers are too reluctant to extract [small] classes.

That’s what’s hard about software, is responding to change. I saw a keynote by Dave Thomas, and he said “The only thing worth worrying about when you look at code is: is it easy to change?”

Advertisements

A therapy session with Katrina Owen

This week I’m posting a mini-series on great conference talks.

Today’s installment is a talk by Katrina Owen called Therapeutic Refactoring.

I’ve watched and re-watched, and re-re-watched this talk several times over the past year. Each time, it just seems to get better and better. I understand more of it, and notice and learn new things.

It’s also got great slides, a lot of which are from Hyperbole and a Half.

Katrina is also running exercism.io, a site where you can complete small coding problems and get peer feedback on them. I’ve been enjoying participating in that recently as well.

Enjoy the talk!

Moving grouping queries from the Database to ActiveRecord

Today’s post is a refactoring story. The ERP I wrote for my old company includes an automatic commission calculator. It looks at quite a few aspects:

  • How much we billed each client
  • What ‘category’ each billed service was
  • Whether the employee was involved in the initial sale or ongoing management of the client at the time the invoice was issued
  • Whether the client reimburses our expenses separately from the cost of the service, or whether the expenses are ‘baked in’ to the cost of the service
  • Whether we have received payment yet, or are still waiting to recieve payment
  • Whether we have already met certain limits on how much commission we pay per client, per employee

The result is a very complex database view commission_lines that has an ActiveRecord class CommissionLine built on top of it.

In addition, there were three more database views and corresponding ActiveRecord classes to get a summarized monthly report, monthly totals, and all time totals. When refactoring, I got to delete a good portion of that code. Here’s how:

I recently learned more about ActiveRecord’s ability to do grouping queries, and moved those three totals queries from the database into class methods on CommissionLine, like this:

  class CommissionLine < ActiveRecord::Base

    def self.monthly_report_lines
      group_and_order = 'financial_month, contact_en_name, client, category, global_rate, client_manager_rate, sales_rate'
      select('financial_month, contact_en_name, client, category, sum(line_total) billed_total,
              global_rate, client_manager_rate, sales_rate,
              sum(global_amount) global_amount, sum(client_manager_amount) client_manager_amount, sum(sales_amount) sales_amount,
              sum(commission_line_total) commission_total, sum(settled_commission_line_total) settled_commission_total')
      .group(group_and_order)
      .order(group_and_order)
    end

    def self.monthly_totals
      select('financial_month, contact_en_name, sum(commission_line_total) commission_total, sum(settled_commission_line_total) settled_commission_total')
      .group('financial_month, contact_en_name')
      .order('financial_month, contact_en_name')
    end

    def self.all_time_totals
      select('contact_en_name, sum(commission_line_total) commission_total, sum(settled_commission_line_total) settled_commission_total')
      .group('contact_en_name')
      .order('contact_en_name')
    end
  end

There are a few benefits I see to doing this refactoring:

  • The logic of how we total things is more visible to other developers
  • It creates opportunities to further refactor the three class methods to be DRYer
  • It is easy to make changes to the queries without db migrations
  • It is easy to chain on other scopes. For instance, we could get totals of just half a month if we wanted, instead of being forced to look at totals from an entire month at a time.
  • It resulted in 106 less lines of code all together