A Post Entitled New Year’s Cleanup: Compact Is Not Necessarily Concise
To start the New Year off I am having the distinct pleasure of cleaning up bunch of old code. Aside from my beautiful code (ahem) I am also having the chance to review some other developers and that is leading me to some thoughts about style.
Style Lesson 1: Compact is Not Necessarily Concise
One of the great things about Ruby is it’s concise syntax. Well-written Ruby code is and should be short and to the point. Unfortunately sometimes the notion of ‘short’ gets confused for ‘concise’. They are not the same thing. Take, for example, the following deprecated code that I ran across:
class << self
def complete_payment(panel_id)
Panel.transaction do
panel = Panel.find(panel_id)
court_ap_id = Court.find(panel.court_id).accounts_payable.id
# identify all pending expenses for the panel
pending_expenses = Financial::RosterExpense.for_panel(panel_id).for_state('approved')
# collect hash fo summarized rosters to send for journalling to financials
collection_of_rosters_for_journal_transaction =
# convert array of hashes to a single hash
Hash[
# first group by pay_type
*pending_expenses.group_by(&:pay_type).collect do |pay_type, roster_by_pay_type|
# then sub group by payable_to
roster_by_pay_type.group_by(&:payable_to).collect do |payable_to, roster_by_payable_to|
self.financial_transaction_package(pay_type, payable_to, roster_by_payable_to.map(&:amount).sum, roster_by_payable_to.size, User.current_user.id, panel.court_id, court_ap_id)
end
end.flatten.collect{|h| h.to_a}.flatten
]
resp = Journal.post_group_of_entries_with_summary_account(collection_of_rosters_for_journal_transaction, panel.court_id, User.current_user.id)
unless resp.instance_of?(Net::HTTPOK)
logger.debug("ERRORED in Journaling with #{resp.body}")
raise ActiveRecord::Rollback
response = false
end
end
response ||= true
return response
end
end
W O W. There’s a lot going on there, no? It is arguably short but not at all concise. Concise has to do just as much with readability as it does with brevity. Concise makes things clear and obvious, especially for those of us who get to read what you’ve left behind. So let’s take another stab at that in a way that’s hopefully more ‘concise’.
Known knowns
One of the first things that strikes me is the line just below the comment ‘identify all pending expenses for the panel.’ What is not obvious in this code snippet is that this query is repeated several times so it needs drying up. Further more, note the actual query. Is it really checking for ‘pending’ things? No, it’s looking for ‘approved’ things. Duplicated code often does that. Let’s extract a function there.
class << self
def self.approved_expenses_for( panel_id )
for_panel( panel_id ).for_state( 'approved' )
end
end
Simple enough. We’re already working in the Financial::RosterExpense class and the method in question is a class-level method so we can dispense with the class reference. We can also give the query a descriptive name that matches what it returns. In this case I’ve left off the word ‘panel’ from the end of the method name simply because I like the way it will read when used: ‘approved_expenses_for panel_id’. If that does not appeal to you, no big deal.
Wielding the Unweildy
Take a deep breath and grab a cup of coffee before continuing. We’re going to tackle that big chunk in the middle.
The heart of the Panel.transaction block is simply building up a hash. The syntax may be a bit unfamiliar and hard to decipher. One way to instantiate a hash is to call Hash[] passing it an array. Hash responds by zipping the array in pairs and returning a hash in which the first, third, fifth, … entries are the keys and the second, fourth, sixth, … entries are the values of the hash. This somewhat infrequent use of Hash is all the more obscured by the fact that the square brackets in the call are separated by a huge code block. Obviously this needs to be broken down in a way that makes that intention clearer.
Delving inside the Hash call we see that the expenses are grouped twice (once by pay_type and then by pay_to) and the innermost group is used to call a helper function that returns a hash. Interestingly, the hash is collected up into an array at each level. At the end, the nested arrays are flattened, with the inner hash being converted from a hash to an array in the process. I don’t know why.
Private, get me that expense report
The first step in cleaning up this code will be to create another helper function that performs the nested grouping and summary work. Within the helper function we’ll add some variables to make some of the calculations more explicit in case we need to modify those calculations in the future.
class << self
def approved_expenses_for( panel_id )
for_panel( panel_id ).for_state( 'approved' )
end
def build_expense_summary( expenses )
expenses.group_by(&:pay_type).collect |pay_type, expenses_by_pay_type|
expenses_by_pay_type.group_by(&:payable_to).collect do |payable_to, pay_to_expenses|
payment_amount = pay_to_expenses.map(&:amount).sum
payment_count = pay_to_expenses.size
self.financial_transaction_package(pay_type, payable_to, payment_amount, payment_count, User.current_user.id, panel.court_id, court_ap_id)
end
end
end
end
It turns out that the ‘financial_transaction_package’ helper method is returning a very simple hash. The key to the hash is a unique id and the value is a hash of transaction attributes associated with that id. This is useful to know because we can merge these simple hashes together without worrying that we might replace values. In fact, we can perform the merges at multiple levels without worrying about losing data. As a result, we can transform the collect’s into inject’s and build the array from the inside out.
class << self
def approved_expenses_for( panel_id )
for_panel( panel_id ).for_state( 'approved' )
end
def build_expense_summary( expenses )
expenses.group_by(&:pay_type).inject({}) |final_expenses, (pay_type, expenses_by_pay_type)|
package = expenses_by_pay_type.group_by(&:payable_to).inject({}) do |temp_expenses, (payable_to, pay_to_expenses)|
payment_amount = pay_to_expenses.map(&:amount).sum
payment_count = pay_to_expenses.size
expense_package = self.financial_transaction_package(pay_type, payable_to, payment_amount, payment_count, User.current_user.id, panel.court_id, court_ap_id)
temp_expenses.merge( expense_package )
end
final_expenses.merge( package )
end
end
end
The extra statements that the inject requires is making that a little cluttered. Let’s break the build_expense_summary up into two functions, one that builds the expenses by pay_type and another that builds the expenses by payable_to. Since their scope is pretty specific we’ll make them private, too.
class << self
def approved_expenses_for( panel_id )
for_panel( panel_id ).for_state( 'approved' )
end
private
def build_expense_summary( expenses )
expenses.group_by(&:pay_type).inject({}) |expense_package, (pay_type, expenses_by_pay_type)|
package = build_financial_transaction_package_by_payable(pay_type, expenses)
expense_package.merge( package )
end
end
def build_financial_transaction_package_by_payable(pay_type, expenses)
expenses.group_by(&:payable_to).inject({}) do |expense_package, (payable_to, pay_to_expenses)|
payment_amount = pay_to_expenses.map(&:amount).sum
payment_count = pay_to_expenses.size
package = self.financial_transaction_package(pay_type, payable_to, payment_amount, payment_count, User.current_user.id, panel.court_id, court_ap_id)
expense_package.merge( package )
end
end
end
As constructed, build_expense_summary will return a hash. Thus, with this refactoring we’ve not only made the intention of the nested group_by calls more explicit we’ve also eliminated the need for the less-common mode of Hash instantiation. Nice.
We have also substantially simplified the original complete_payment method. The call to build up the set of expenses to process is wrapped up in a method and the processing of those expenses is also a method with an intention-revealing name.
Before presenting the final result I’ll also point out that I have a preference for methods that exit once they’ve determined they’ve reached their end. In this case I would much prefer to return false when the HTTP request fails than to set a flag and exit later.
The Reveal
class << self
def complete_payment( panel_id )
panel = Panel.find(panel_id)
court_ap_id = Court.find(panel.court_id).accounts_payable.id
transaction do
approved_expenses = approved_expenses_for panel_id
expense_summary = build_expense_summary approved_expenses
unless Journal.post_group_of_entries_with_summary_account(expense_summary, panel.court_id, User.current_user.id).is_a?(Net::HTTPOK)
logger.debug("ERRORED in Journaling with #{resp.body}")
raise ActiveRecord::Rollback
end
end
end
def approved_expenses_for( panel_id )
for_panel( panel_id ).for_state( 'approved' )
end
private
def build_expense_summary( expenses )
expenses.group_by(&:pay_type).inject({}) |expense_package, (pay_type, expenses_by_pay_type)|
package = build_financial_transaction_package_by_payable(pay_type, expenses)
expense_package.merge( package )
end
end
def build_financial_transaction_package_by_payable(pay_type, expenses)
expenses.group_by(&:payable_to).inject({}) do |expense_package, (payable_to, pay_to_expenses)|
payment_amount = pay_to_expenses.map(&:amount).sum
payment_count = pay_to_expenses.size
package = self.financial_transaction_package(pay_type, payable_to, payment_amount, payment_count, User.current_user.id, panel.court_id, court_ap_id)
expense_package.merge( package )
end
end
end
Conclusion
The refactored code is both longer and more concise. Huh? Well, in this case we’ve actually added a half-dozen lines or so to the solution (counting comments and blank lines in each). We’ve also expanded one method into four. In those senses the code is longer. At the same time the methods that we have are significantly shorter with method names and additional variables used to make it more explicit. The methods are more concise to read and easier to comprehend. It’s also more concise in the sense that at least one of the extracted methods will be reused elsewhere so the code as a whole is more concise.
