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 endA few things stood out:
- There is a locations hash
- The
each
iterator 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 endEverything 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 endThe 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.