931970b8dc51b72e05e3a12b88612d61

I have a method called ranking in the Item class which will get all items with their ranking value as well as the vote for the current user if present.

The two methods raking and user_vote use these retrieved values if present and if not, they retrieve them themselves. That's why a lot of the calculated-on-sql value names are set as class variables. I'm including those two methods as well.

Is there any way to improve this code? It looks just ugly. Building SQL strings, joins and so on that way.

Thanks.

PS: you can see it in action in http://isitsciencefiction.com/items

class Item < ActiveRecord::Base
  ALL_VOTES = "all_votes"
  SCORE = "score"
  USER_VOTES = "user_votes"
  USER_VOTE = "user_vote"
  VOTER_ID = "voter_id"
  
  belongs_to :recommender, :class_name => "User", :foreign_key => "recommender_id"
  has_many :votes, :as => :voteable
  has_many :reasons
  belongs_to :item_type
  
  def self.ranking(user_id)
    select = ["#{Item.table_name}.*", "IFNULL(sum(#{ALL_VOTES}.value), 0) as #{SCORE}"]
    joins = [<<-eos]
      LEFT JOIN #{Vote.table_name} as #{ALL_VOTES} ON
        #{ALL_VOTES}.voteable_id = #{Item.table_name}.id and
        #{ALL_VOTES}.voteable_type = "#{Item.name}"
    eos
    group = [:id]
    if user_id
      select << "#{USER_VOTES}.value as #{USER_VOTE}"
      select << sanitize_sql_array(["? as #{VOTER_ID}", user_id])
      joins << sanitize_sql_array([<<-eoq, user_id])
        LEFT JOIN #{Vote.table_name} as #{USER_VOTES} ON
          #{USER_VOTES}.voteable_id = #{Item.table_name}.id and
          #{USER_VOTES}.user_id = ? and
          #{USER_VOTES}.voteable_type = "#{Item.name}"
      eoq
      group << "#{USER_VOTES}.value"
    end
    
    Item.find(:all,
      :select => select.join(", "),
      :joins => joins,
      :group => group.join(", "),
      :order => "score DESC")
    
    # This is the query it should generate (for user_id = 2)
    # SELECT items.*,
    #        user_votes.value as user_vote,
    #        IFNULL(sum(all_votes.value),0) as score,
    #        "2" as voter_id
    # FROM items
    # LEFT JOIN votes as all_votes ON
    #     all_votes.voteable_id = items.id and
    #     all_votes.voteable_type = "Item"
    # LEFT JOIN votes as user_votes ON
    #     user_votes.voteable_id = items.id and
    #     user_votes.user_id = "2" and
    #     user_votes.voteable_type = "Item"
    # GROUP BY items.id, user_votes.value
    # ORDER BY score DESC
  end
  
  def score
    s = read_attribute(SCORE)
    if s == nil
      votes.sum :value
    else
      Integer(s)
    end
  end

  def user_vote(user_id)
    if user_id.blank?   # If user_id is blank, there's no user
      return 0          # the vote is 0
    else
      # If we have a VOTER_ID attribute and it's equal to the user_id
      if Integer(read_attribute(VOTER_ID)) == user_id
        # we already have the vote value in the USER_VOTE attribute,
        return Integer(read_attribute(USER_VOTE))
      else
        # otherwise fetch it from the database and return it or zero if not found
        vote = votes.find(:first, :conditions => ["user_id = ?", user_id])
        if vote
          return vote.value
        else
          return 0
        end
      end
    end
  end
  #...
end

Refactorings

No refactoring yet !

Your refactoring





Format Copy from initial code

or Cancel