Average days on market calculation I have the following method which calculates average days on market for a sale type segment:

def average_days_on_market_sale_type_segment
    # on market days
    on_market_days = {}

    # 1. get last year sales - note that the dates need to be sequential in time (oldest in time comes first, most recent in time commes last)
    last_year_sales = self.real_property_sale_projects.where(status: 'reviewed')
      .where('deal_date between ? and ?', Time.at(Time.now - 1.years), Time.now)
      .map(&amp;:real_property_sale)

    # 2. loop each sale type, convert sale description string to symbol
    last_year_sales.each do |sale|
      sd = sale.sale_description.parameterize.underscore.to_sym

      on_market_days[sd] ||= { count: 0, days: 0 }

      on_market_days[sd][:count] += 1

      # get the days on market value
      on_market_days[sd][:days] += sale.real_property_sale_project.days_on_market
    end

    # 3. summarize average
    summary = {}

    on_market_days.each do |k, v|
      summary[k.to_s.humanize] = (v[:days].to_f / v[:count].to_f).round
    end

    # 4. return summarized result
    # it should contain each sale description type as a key and average day as a number
    summary
  end


Is there a more elegant way to achieve the same thing? Does the solution contain any performance issues/gotchas?
Main notes:


Whenever you have a method broken up into commented sections, consider pulling out each section into its own, clearly named method.  This will make the pieces of your code focused and readable, and eliminate the need for comments.
Use ruby's built in functional methods rather than procedurally looping and bulding things manually.  group_by does most of the work for you in this case.
You can map over hashes, returning an array of [key, value] arrays, and then call to_h at the end to get back a transformed hash.
average_days_on_market_sale_type_segment is an unwieldy name.  avg_days_on_market_by_type seems like a small improvement you could make (generally naming hashes as value_by_key is clearer), but it's still long.  If you have expertise in real estate, you might be able to come up with something clearer that reflects language used by agents.  A quick google search came up with "CDOM", so maybe cdom_by_type would work, but with naming, there's no "correct" answer -- only what will be clear to the people working with the system.


A couple other things:


This isn't tested, since I don't have your data to test with.  So there may be some trivial errors.
The methods sales_description and avg_days_on_market that I introduced aren't really instance methods, since they are pure functions operating on a sale or array of sales.  This hints at further opportunities for refactoring by introducing objects for Sale or Sales, but that may be overkill at this point.


Refactored code:

def average_days_on_market_sale_type_segment
  last_years_sales.group_by {|sale| sales_description(sale)}
                  .map {|desc, sales| [desc, avg_days_on_market(sales)]}.to_h
end

def last_year_sales
  self.real_property_sale_projects
  .where(status: 'reviewed')
  .where('deal_date between ? and ?', Time.at(Time.now - 1.years), Time.now)
  .map(&amp;:real_property_sale)
end

def sales_description(sale)
  sale.sale_description.parameterize.underscore.to_sym
end

def avg_days_on_market(sales)
  total_days = sales.map {|sale| sale.real_property_sale_project.days_on_market}
                    .inject(:+)
  total_days / sales.size
end