More Idiomatic Ruby

Posted by koz Sunday, January 21, 2007 22:47:00 GMT

Carl Fyffe submitted a small snippet of an existing application, asking how we’d polish it. We’ve decided to try out a new format for this kind of micro-refactoring; let us know what you think.

Before:

1
2
3
4
5
6
7
8
9
10
11
12

# returns Hash of Engagements keyed on date.
def self.find_by_year_month(year, month)
  date = Time.gm year, month
  engagements = {}
  all = find(:all, :conditions => ["start >= ? and start <= ?", date.last_month.end_of_month, date.next_month.beginning_of_month], :order => :start)
  all.each do |e|
    key = e.start.strftime('%Y%m%d')
    engagements[key] = [] unless engagements.has_key?(key)
    engagements[key].push e
  end
  engagements
end

After:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36

def self.find_by_year_month(year, month)
  # We're dealing with Dates, so let's use the Date class. Date can
  # be a little slower than Time, but as this method isn't going to
  # perform thousands of operations per request, we'll be fine.

  requested_date = Date.new(year, month, 1)

  # The range in the original code went from the last day of the 
  # previous month, to the first day of the following month.

  # Date#- subtracts days from a given date, which lets us find
  # the end of last month.

  from = requested_date - 1

  # Date#>> lets us advance requested date by one month, 
  # giving us the first of the next month.

  to =  requested_date >> 1

  # The BETWEEN operator lets you simplify the SQL, and it more 
  # clearly matches the intent.

  engagements = find(:all, :conditions => ["start BETWEEN ? and ?", from, to],
                           :order      => :start)

  # Enumerable#group_by returns a hash with the date strings as keys
  # and an array of the corresponding engagements as the values.
  # If you find yourself initializing a collection, adding some items
  # then returning it, you can probably find a method to tidy that
  # up.

  engagements.group_by do |engagement|
    engagement.start.strftime("%Y%m%d")
  end
end