Weekly refactoring lessons – part 1
I decided a few weeks ago to write a weekly reference about lessons learned. The lessons may not necessary be new or ground breaking, but further research into a subject where refactorings can be made. I hope this will help others in the process.
Today, I have 2 refactorings to note:
* note – For ease in understanding, I will be writing all code in Ruby.
(Rails) Enumerable.inject
Iterating collections in many languages can be clumsy and code verbose. For instance, you may see the following code written in the java world. It requires a developer to specifically define a return array, add to it while iterating, and return the value. Each step being a separate line.
def adjective_products
// define a method to return an array of adjective prefixed products
adjectives = ["fruity", "frothy", "relaxing"]
product = "beverage"
product_names = []
for adj in adjectives
product_names << "#{adj} #{product}"
end
product_names
end
In Ruby there are many changes that can be made to this code. Let's start by refactoring the for loop from of the equation and replace it with a block. It is not very ruby-like and makes my head hurt as though we are using java.
def adjective_products
// define a method to return an array of adjective prefixed products
adjectives = ["fruity", "frothy", "relaxing"]
product = "beverage"
product_names = []
adjectives.each { |adj| product_names << "#{adj} #{product}" }
product_names
end
That's better... at least for now. We have eliminated two lines of code and maybe even made the code seem more readable? However, we still are required to setup an initial variable, iterate the collection capturing values, and finally return that value. In the next refactoring, we will remove the need for this variable by taking advantage of the inject method on the adjectives array.
def adjective_products
// define a method to return an array of adjective prefixed products
adjectives = ["fruity", "frothy", "relaxing"]
product = "beverage"
adjectives.inject([]) { |acc, adj| acc << "#{adj} #{product}" }
end
To better understand what is happening, let's break down the statement.
First: create a new variable to be returned
Where previously we were defining a product_names array, we are now passing it as a parameter to the inject method
Second: iterate a loop with conditional logic.
The only difference here is the use of the acc, or accumulator, inside inject that is a reference to the array parameter passed into the method.
Third: return a value
From what we already know in ruby, statements will always return the last value set. So the accumulator is being set with each iteration, returning itself. And upon completion of inject, the method will return that accumulator.
Easy enough eh?
(Rails) Range in reverse
This week I had a need to iterate over several already defined columns from a fastercsv row in reverse and return an array of the values. While it can be accomplished in a more declarative way, I wanted to see if it could be done programmatically. So off to Range world I went, starting with the code below:
# comma delimited data
# role_1, role_2,role_3,role_4,role_5
# Admin,System,File
# Admin,System,Dir
# Admin,Web,Save
file_path = "/my/comma/delimited/file"
FasterCSV.foreach( file_path, {
:headers => true, :skip_blanks => true, :header_converters => :symbol
} ) do |row|
row_array = []
row_array << row[:role_5]
row_array << row[:role_4]
row_array << row[:role_3]
row_array << row[:role_2]
row_array << row[:role_1]
row_array.compact!
# persist the row_array code here (replaced with puts)
puts row_array.inspect
end
The idea was to iterate rows of comma delimited rows, retrieve a preset number of columns, compact the nils out, and then persist the role data.
So let's start off by removing the declarative code and replace it with a iteration to reduce code.
# comma delimited data
# role_1, role_2,role_3,role_4,role_5
# Admin,System,File
# Admin,System,Dir
# Admin,Web,Save
file_path = "/my/comma/delimited/file"
FasterCSV.foreach( file_path, {
:headers => true, :skip_blanks => true, :header_converters => :symbol
} ) do |row|
row_array = []
(5..1).each do |i|
row_array << row[:"role_#{i}"]
end
row_array.compact!
# persist the row_array code here (replaced with puts)
puts row_array.inspect
end
With that finished, the code should work properly right? Well yes, but unfortunately there is a bug in the way it is written. Notice the range value is from 5 to 1. That is the way the declarative code worked, but the Range object does not. Range objects require a lessor value to a greater value if you want to_a (array) to return anything other than empty.
Try the following for example:
$ irb
irb(main):001:0> (1..5).to_a
=> [1, 2, 3, 4, 5]
Now in reverse:
$ irb
irb(main):002:0> (5..1).to_a
=> []
Oops! What went wrong exactly? Well, Ruby does not iterate in reverse with ranges. They are counter intuitive, but there are two options. I would suggest the later, since it is less to write and definitely more clear as to what is happening.
$ irb
irb(main):004:0> (-5..-1).to_a.collect { |r| r.abs }
=> [5, 4, 3, 2, 1]
$ irb
irb(main):006:0> (1..5).to_a.reverse
=> [5, 4, 3, 2, 1]
Finally to clean up the original code, here is the code after the refactoring. Notice the use of inject too!
# comma delimited data
# role_1, role_2,role_3,role_4,role_5
# Admin,System,File
# Admin,System,Dir
# Admin,Web,Save
file_path = "/my/comma/delimited/file"
FasterCSV.foreach( file_path, {
:headers => true, :skip_blanks => true, :header_converters => :symbol
} ) do |row|
# persist the row_array code here (replaced with puts)
row_array = (1..5).to_a.reverse.inject([]) { |acc, i|
acc << row[:"role_#{i}"]
}.compact!
# persist the row_array code here (replaced with puts)
puts row_array.inspect
end
Depending on your level of comfort, this refactoring may not be your cup of tea. While neither manner is absolutely correct, I prefer it over the verbose alternative. It is a decision that is best made by the team. The more readable code is to future eyes, the easier it is to maintain.
Here is a quote from Martin Fowler discussing the use of closure methods like inject:
Now there's a serious question here about a method that complex. (And I must admit I used to be wary of chaining like this.) But it's a good illustration of how useful it is to string these methods together. (And in a language that supports the right kind of concurrency, it has important implications for multi-processor performance.)
Thank you for reading!
