views:

50

answers:

3

I have a collection of Episodes which is connected to a Season and a Show.
I need to display them as such:

Show title
....Season number 1
........Episode name
........Episode name
....Season number 2
........Episode name
........Episode name

My controller:

  def index
    @show_ids = Following.find_all_by_user_id(current_user.id).collect(&:show_id)
    @seen_ids = SeenEpisode.find_all_by_user_id(current_user.id).collect(&:episode_id)
    if @seen_ids.any?
      @episodes = Episode.find(:all, :conditions => ["show_id IN (?) AND episodes.id NOT IN (?)", @show_ids, @seen_ids], :joins => [:show, :season])
    else
      @episodes = Episode.find(:all, :conditions => ["show_id IN (?)", @show_ids], :joins => [:show, :season])
    end
  end

My view:

<ul>
<% @episodes.group_by(&:show).each do |show, episodes| %>
  <li><h2><%= show.name %></h2></li>
  <% episodes.group_by(&:season).each do |season, episodes| %>
    <li><strong><%= season.number %></strong></li>
    <% episodes.each do |episode| %>
      <li><%= episode.name %></li>
    <% end %>
  <% end %>
<% end %>
</ul>

This works fine, although I know this is not a good method, and the performance is SHIT (like 10 seconds for about 150 records). How can I have a grouping like this with good performance?

Also, how can I refactor this?

if @seen_ids.any?
  @episodes = Episode.find(:all, :conditions => ["show_id IN (?) AND episodes.id NOT IN (?)", @show_ids, @seen_ids], :joins => [:show, :season])
else
  @episodes = Episode.find(:all, :conditions => ["show_id IN (?)", @show_ids], :joins => [:show, :season])
end
+2  A: 

using the "NOT IN" clause is generally slow. Instead left join on the SeenEpisode table and add a condition where SeenEpisode is NULL

Episode.find(:all, :joins => "LEFT JOIN SeenEpisode ON SeenEpisode.show_id = Episode.show_id", :conditions => "SeenEpisode.show_id IS NULL")

Note that I omitted some of the clauses for clarity. What this does is keep all records from Episode and add in columns from SeenEpisode that match. The condition then takes out those matching records.

Zaid Zawaideh
Actually its the group_by methods that makes the page slow. Without the view code, the page loads in 100ms instead of 10sec.
Frexuz
Do you have indices on your database (created in your migrations) on Show and Season?
Zaid Zawaideh
A: 

I noticed that my db got queried Select * from shows Where Id = 100 for each record in the loop (show.name). I'm guessing the join did not work because of ambiguous named columns (episodes.name and shows.name)

This is what I ended up with.

query:

@episodes = Episode.find(:all, :select => "episodes.*, shows.name AS show_name", :conditions => ["show_id IN (?) AND seen_episodes.episode_id IS NULL", @show_ids], :joins => "INNER JOIN shows ON shows.id = episodes.show_id LEFT JOIN seen_episodes ON seen_episodes.episode_id = episodes.id")

view:

<ul>
<% @episodes.group_by(&:show_name).each do |show_name, episodes| %>
  <li><h2><%= show_name %></h2></li>
  <% episodes.group_by(&:season_number).each do |season_number, episodes| %>
    <li><strong><%= season_number %></strong></li>
    <% episodes.each do |episode| %>
      <li><%= episode.name %></li>
    <% end %>
  <% end %>
<% end %>
</ul>

Also, I already had the season_number in a "cache column" on each episode.

I think this is OK. The query is not very pretty, but at least I like the result:

Completed in 109ms (View: 31, DB: 15)

Frexuz
+2  A: 

First, make sure your database has indexes on any foreign key columns you're querying against (I generally index anything_id as a matter of course:

add_index :episodes,   :show_id
add_index :followings, :user_id

To clean up your finds, try something like this (from your updated post):

@episodes = Episode.scoped(
              :conditions => ["show_id IN (?)", @show_ids],
              :include => :show )
if @seen_ids.present?
  @episodes = @episodes.scoped(
              :conditions => "seen_episodes.show_id IS NULL",
              :joins => :seen_episodes )
end

The above assumes you're using Rails 2 (since you were using the .find(:all) syntax...) but you can clean that up further by using .where, etc. instead of .scoped if you're on Rails 3.

Andrew Vit
A very good tip! Thanks!
Frexuz
The above scoping can also be done with named scopes, which would move that sql logic into the model. I think that's a bit beyond the scope of this question, but I recommend you look it up.
Andrew Vit