2
\$\begingroup\$

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(&: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?

\$\endgroup\$
3
\$\begingroup\$
  1. Use 1.year.ago to get the start of the time range.

  2. Create a method on the sale model that returns the description in the form you want (i.e. underscored symbol). On the other hand, is it necessary? You turn it back into a string afterward. If you're relying on parameterize + underscore to "normalize" slightly different strings that mean the same, you should probably normalize your database with a migration instead. If the strings are already normalized, just use 'em directly.

  3. Use #group_by to group by sales type.

  4. Use #fdiv if you want to avoid integer math.

The record-fetching a bit complex, but we'll get to that. Looking just at the average calculation, you can do this instead:

averages = last_year_sales
  .group_by(&:description) # I'm assuming that there's no real need to symbolize the description
  .map do |description, sales|
    sum = sales.map { |sale| sale.real_property_sale_project.days_on_market }.reduce(:+)
    [description, sum.fdiv(sales.count)]
  end

Hash[averages]

Overall though, your database seems to be laid out in a way that makes this query extra tricky.

You're fetching all associated real_property_sale_projects (I'll just say "projects"), and then - for each of those - you fetch an associated real_property_sale? But when you have to do the averaging, the sale doesn't contain all the necessary information - the days_on_market number is attached to the project.

And you're doing all this in a 3rd model altogether it seems. So that's pretty complex.

However, you can also do it all in the database - it's exceedingly good at aggregating data. For instance, this, I think, should work:

self.real_property_sale_projects
  .joins(:real_property_sale)
  .where(status: "reviewed")
  .where("real_property_sale_projects.deal_date between ? and ?", 1.year.ago, Time.now)
  .group("real_property_sales.description")
  .average(:days_on_market)

That should give you the result. There may be a nicer way to write it.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the response - very much appreciated! I'm going to mark this as correct purely for the elegance of doing it all in the database. However, I'm unsure what the .joins method does? I've read the documentation but I am still unsure. \$\endgroup\$ – Thomas Taylor Nov 18 '15 at 2:09
  • \$\begingroup\$ @ThomasTaylor It joins two tables in the database, so the query can look at both simultaneously. In this case, we need some info from the projects tables, and some from the sales tables, so they're (in effect) combined when the query is performed. For more, see wikipedia's article on SQL joins - it's core feature of relational databases like the ones Rails use; Rails just exposes a simple joins method to use it \$\endgroup\$ – Flambino Nov 18 '15 at 16:47
3
\$\begingroup\$

Main notes:

  1. 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.

  2. 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.

  3. 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.

  4. 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:

  1. This isn't tested, since I don't have your data to test with. So there may be some trivial errors.

  2. 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(&: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
\$\endgroup\$
  • \$\begingroup\$ You can remove the variable assignment in last_year_sales \$\endgroup\$ – Mark Thomas Nov 17 '15 at 21:24
  • \$\begingroup\$ Thanks for the response! I've tested the above and the first method returns a syntax error: syntax error, unexpected '|', expecting '}' .map do {|desc, sales| [desc, avg_days_on_market(sales)]}.to_h \$\endgroup\$ – Thomas Taylor Nov 18 '15 at 2:16
  • \$\begingroup\$ There was both a do and a { ... I fixed it. \$\endgroup\$ – Jonah Nov 18 '15 at 3:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.