How to simplify enum-ridden method that sums monthly balances

Question

I'm trying to refactor an really poorly written method. The method is meant to scan an array containing balance hashes that look like this:

{ amount: $123, month_end: '2013-01-31' }

and return all the balances with the same month end and then sum up the amounts.

def monthly_total(month)
  balances = [ balance_1, balance_2 ]
  # get and array of balances
  balances_for_month = balances.select do |balance|
    balance.month_end == month
  end
  # grab only the balances for the desired month
  balance_amounts = balances_for_month.map do |balance|
    balance.amount
  end
  #take all the balances for the month and sum them.
  balance_amounts.inject{|sum,x| sum + x }
end

But there's gotta be a sleeker way to do this. How can I restructure this method so that it loops over the original array once time, instead of creating new array and looping through them?

Answer 1

That's good code. The variable names are good and the intent is obvious.

There's nothing wrong with going through the array twice. Ruby is more about making your intent clear than it is about the fastest possible code. You should only optimize when the code is known to not be fast enough, and then you should measure, to make sure you're actually making it faster. Ruby will often surprise you by being slower when you do something you think will make it faster.

The second loop (transform balance into balance.amount) can be shorted to:

balances_amounts = balances_for_month.map(&:amount)

The sum can be shortened to:

balance_amounts.inject(&:+)

See What does to_proc method mean? for an explanation of how these work.

Sometimes temporaries add to the clarity of the code; sometimes not. Once you're using the techniques above, the temporaries can be gotten rid of, leaving:

balances.select do |balance|
  balance.month_end == month
end.map(&:amount).inject(&:+)

This might seem a little dense at first, but becomes clear once one is familiar with these idioms.