CODEWITHSUNDEEP
ruby-on-railsrubyruby-on-rails-3
Gshaw
Gshaw

Reputation: 47

Ruby on Rails: Is this the right way to structure code in Ruby on Rails? This is too slow

I am not very familiar with RoR but my colleague has written this code for one of our applications & I have a feeling this is just too un optimized and too many queries for a simple task.

We have 3 tables. Artist, Album & Song. Artist can have multiple albums. Album can have multiple songs. We are trying to output the top 10 songs based on a field popularity_total in the songs table. Then there are other tables that capture likes etc.

def top
     # a list of 10 most played songs in the past week
     @toplistsongs =  Song.select(INDEX_COLUMNS).order("popularity_total,created_at     DESC").limit(10)
     @toplistsongs.each do |song|
      song['urls'] = song.get_song_urls
      song['artist'] = song.get_song_artist
      song['genre'] = song.tag_list
      song['hearts'] = song.likers(User).count
      song['like_status'] = ( current_user!=nil ? current_user.likes?(song) : false )
      song['impressions'] = song.impressionist_count
      song['albums'] = song.get_song_album
    end

    @w = {'top' => 
             {  
                'song' => @toplistsongs
             }
        }
     respond_with(@w)
end

Every fetch inside the loop with result in a hit on the db. I have a feeling there are too many queries happening in the loop for each song when ideally they can all be done using single query for all the songs. Can anyone suggest if this is a standard rails way of handling things or is this outright bad code?

Thanks for all the help.

Upvotes: 2

Views: 166

Answers (2)

Samiron
Samiron

Reputation: 5317

This is just a quick reply. Let me know if you have any confusion about anything in particular.

Your model definitions should be like this

class Artist<AR::Base
    has_many :albums
end

class Album<AR::Base
    belongs_to :artist
    has_many :songs
end

class Song<AR::Base
    belongs_to :album
end

Your find line should be like this

@toplistsongs = Song.order("popularity_total, created_at").limit(10).includes(:tags, { :songs => :artists }, :urls, :likers)

should return top 10 songs based on your popularity_total field.

Thnx to Erez Rabih for the include part

Now

  • a song.album should return the Album of the Song.
  • a song.album.artist should return the Artist of the Song.

you can load these relationships by eager loading. Follow the link.

Assuming you have relationships with tags and urls.

Upvotes: 0

Erez Rabih
Erez Rabih

Reputation: 15788

You can use ActiveRecord includes method in order to reduce DB queries count. For example:

 @toplistsongs =  Song.select(INDEX_COLUMNS).order("popularity_total,created_at     DESC").limit(10).includes(:tags, { :songs => :artists }, :urls, :likers)

This will produce 5 queries for the whole top method and is not dependent on the number of songs you want to present. In other words the number of queries is constant.

You will have to use ActiveRecord associations for this to work. I can see that you are using the method get_song_artist I assume it is not made by an association.

To form your relationship in ActiveRecord associations you will have to use the has_many, has_one and belongs_to operators.

Upvotes: 1

Related Questions