2
\$\begingroup\$

I currently have a method that checks if the store is open. It's working nicely, however on the main page I have quite a few stores. Whom again have different locations. Each of these locations again have opening hours.

Now I'm not particularly good with SQL and DB querying and Skylight is complaining and telling me this can most likely be optimized. However I have no clue how.

This is taken from my Store model:

  has_many :business_hours, through: :locations
  ...

  def open?
    if current_business_hour
      true
    else
      false
    end
  end

  def current_business_hour(current_time: Time.current)
      business_hours.where('week_day = ? AND open_at <= ? AND close_at >= ?',
                           current_time.wday, current_time.to_s(:time), current_time.to_s(:time)).first
  end

I'm using Postgres. Now the code should be quite straight forward. I'm looking for business hours that match the current day, and that have a business hour that fits within the current time frame.

This generates:

SELECT  "business_hours".* FROM "business_hours" INNER JOIN "locations" ON "business_hours"."location_id" = "locations"."id" WHERE "locations"."store_id" = ? AND (week_day = ? AND open_at <= ? AND close_at >= ?) ORDER BY week_day LIMIT ?

I'm being notified that:

Skylight detected that this SQL query was repeated multiple times in a single request, which had a significant impact on its response time.

The stores are being rendered like this on a page:

<ul><%= render @stores %></ul>

the _store.html.erb looks like this:

<% if store.open? %>
  <li style="background-image: url(<%= store.header.url %>);">
    <%= link_to store_path(store) do %>
      <h3><b><%= store.name %></b>, <%= store.city.name %></h3>
      <p class="open-store-text">Open</p>
    <% end %>
  </li>
<% else %>
  <li style="background-image: url(<%= store.header.url %>);" class="store-closed">
    <%= link_to store_path(store) do %>
      <h3><b><%= store.name %></b>, <%= store.city.name %></h3>
    <% end %>
    <p class="closed-store-text">Closed</p>
  </li>
<% end %>

Now my goal is to try and improve this.

\$\endgroup\$
  • \$\begingroup\$ If it's being repeated multiple times in a single request, that means you have a n+1 problem. Can you show us where you're calling Store.open? \$\endgroup\$ – Ryan.lay Jun 21 '16 at 15:09
  • \$\begingroup\$ @Ryan.lay I've updated my question with the information you requested! \$\endgroup\$ – Sebastian Jennings Almnes Jun 21 '16 at 16:29
1
\$\begingroup\$

Here's your problem:

<ul><%= render @stores %></ul>

<li><% if store.open? %></li>

You're rendering multiple stores via partials. When each partial is fully rendered out it looks like this:

<ul>
  <li><% if store.open? %></li>
  <li><% if store.open? %></li>
  <li><% if store.open? %></li>
  <li><% if store.open? %></li>
</ul>

Each call to

<% if store.open? %>

will result in a db query, which is where your n+1 problem is coming from.

You have 2 options:

1. Eager loading

#pseudocode
#controller
@stores = Store.includes(:business_hours).where(some_condition:...)

#partial
<% if store.is_open?(store.business_hours) %>

#model
def is_open?(business_hours)
   open = business_hours.detect {|h| (h.wday == Time.now.wday) && ((h.open_time..h.close_time).cover?(Time.now)) }
   open.present? ? true : false
end

May get very memory intensive if you have thousands of stores and many business_hours for each store.

2. Denormalize your database and cache weekdays and business hours in your table

#pseudocode
#migration
table "stores"
   t.integer wday
   t.string open_time
   t.string close_time

#model
def is_open?
  (self.wday == Time.now.wday) && ((self.open_time.to_time..self.close_time.to_time).cover?(Time.now))
end

The approach you take will depend on how many records you need to deal with at any one time.

\$\endgroup\$

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.