Duplication Removal

_description.md

Which version of the code do you find more clear and/or preferable?

balance.rb

module Ledger::Balance

  def balance(transaction=nil)
    sum_of_credits(transaction) - sum_of_debits(transaction)
  end

  def amount_available(transaction=nil)
    self.balance(transaction)
  end

  def spent
    sum_of_debits
  end

protected

  def sum_of_credits(transaction=nil)
    Ledger::Credit.where(destination: self).
      where(transaction ? ['transaction_id <= ?', transaction] : {}).
      select('COALESCE(SUM(credit),0) as credit')[0].credit
  end

  def sum_of_debits(transaction=nil)
    Ledger::Debit.where(source: self).
      where(transaction ? ['transaction_id <= ?', transaction] : {}).
      select('COALESCE(SUM(debit),0) as debit')[0].debit
  end
end

balance_duplication_removed.rb

module Ledger::Balance

  def balance(transaction=nil)
    sum_of_credits(transaction) - sum_of_debits(transaction)
  end

  def amount_available(transaction=nil)
    self.balance(transaction)
  end

  def spent
    sum_of_debits
  end

protected

  def sum_of_credits(transaction)
    sum_of(:credit, transaction)
  end

  def sum_of_debits(transaction)
    sum_of(:debit, transaction)
  end

  def sum_of(type, transaction)
    owner = SOURCE_DESTINATION[type]
    model = "Ledger::#{type.to_s.camelize}".constantize

    model.where("#{owner} = ?", self.id).
      where(transaction ? ['transaction_id <= ?', transaction] : {}).
      select("COALESCE(SUM(#{type}),0) as #{type}")[0].send(type)
  end

  SOURCE_DESTINATION = {
    credit: 'destination_id',
    debit: 'source_id'
  }
end

Which one is more clear? Code Climate gives the first one a grade of B, but the second a grade of A, due to duplication removal.

How would you write this?

Second works well if there's no additional logic to calculate the sum, though I'd probably put sum_of at the top so it 'builds' on each other from the top down.

I think the first one is easier to understand, but there's probably a nicer way to write it. The problem is though you are basically doing a performance optimization here by doing the sum in the database instead of loading everything into memory and summing it there right, and writing code for performance is basically always going to result in less clear code, so you just kind of have to live with that I think if the performance is worth more than the readability (it often is).

If you could just do something like this, it would be easy to read without feeling like there's duplication:

def sum_of_credits(transaction=nil)
    credits_for(transaction).sum
  end

  def sum_of_debits(transaction=nil)
    debits_for(transaction).sum
  end

...but that would mean loading all of the credits/debits into memory and summing them there, instead of chaining the summing operation right onto the query chain before the query gets executed.

Anyways, all trade-offs. I think the first option is nicer, but if you had 3 methods there instead of 2 I would probably say the second is nicer.

The real question I guess is if the way the sum was calculated for one of those changed, would you also be changing the other one? Or might one change and the other stay the same?

If they are always going to change together, then the abstraction makes sense. If it's more of a coincidence that the logic is the same for both right now, and it's likely that if one changed the other might stay the same, then the abstraction is actually going to be a pain in the ass.

@brianhogg I actually prefer to read it in this order. I see that sum_of_credits calls sum_of, and sum_of follows shortly after. Otherwise I don't have context for what sum_of is for.

@adamwathan, I also prefer the first I think. In this case, loading into memory and summing there isn't really an option. Interesting thoughts about how they are calculated though. I can't foresee them changing separately but I guess it is possible. I agree with you about if there were 3 the second option would be nicer. While doing this I had in mind a quote from Sandi Metz that "duplication is cheaper than the wrong abstraction", and I feel like it's pretty applicable here.

But I want my perfect 4.0 GPA on Code Climate! :)