I had to work with this code recently, and it just did not feel right:
class FindsEmployeesByLocations
def self.from(employee_locations=[])
locations = Hash.new
employee_locations.each do |location|
if !locations.key?(location.city)
locations[location.city] = Array.new;
end
locations[location.city].push(location);
end
return locations
end
end
A few things stood out:
- There is a locations hash
- The
eachiterator adds items to the hash based on a condition - Returns the locations hash
Declaring a collection and adding items to it by iterating over the source collection is an infection that people with imperative languages have. With a little bit of functional thinking I was certain this code can be simplified.
Just to be on the safe side I added specs around this before I refactored it:
describe User do
...
context "when the employee locations collection is empty" do
specify "employees by locations is empty" do
expect(FindsEmployeesByLocations.from([])).to be_empty
end
end
context "when there are 3 employees with 2 locations" do
subject { FindsEmployeesByLocations.from(employee_locations) }
it "has only 2 locations" do
expect(subject.keys.count).to eq(2)
end
specify "first locations has 2 users" do
expect(subject.keys.first).to eq("Chicago")
chicago_location = subject["Chicago"]
expect(chicago_location.count).to eq(2)
expect(chicago_location.map(&:user).map(&:name)).to eq(%w(John Kate))
end
specify "last locations has 1 user" do
expect(subject.keys.last).to eq("New York")
new_york_location = subject["New York"]
expect(new_york_location.count).to eq(1)
expect(new_york_location.first.user.name).to eq("James")
end
end
end
Everything passed, I was ready to play with the code.
First I thought I need to use the reduce method. I tried that, but it did not work. Then I started to think about what this piece of code is doing. I realized it's grouping entities based on keys. I was certain Ruby has a group_by method and I was right: there it was.
All that code can be replaced with one beautiful line:
class FindsEmployeesByLocations
def self.from(employee_locations=[])
employee_locations.group_by(&:city)
end
end
The method, or even this class is unneeded for calling something as simple and elegant as Ruby's group_by method.
Be skeptical when you see an empty collection followed by an iterator. If you look closely, you will see that the code can be simpler by using one of Ruby's enumerable functions.
You can find the entire example in this gist.