J. Pablo Fernández wrote:
> Hello Railists,
>
> I have a piece of Ruby on Rails code that has a complex SQL query (well,
> not
> that complex, but as far as I know beyond the ORM capabilities) and for
> my
> taste it has too many strings and harcoded values. I'd like to improve
> it as
> much as possible, so my question is open ended, what else can I do to
> improve it?
In this case, break it into two. See below.
>
> Some particular issues I have
>
> - Is there a way to get a table name to use it in a query in the same
> escaped way as the ORM does? I think this is database independent,
> being `items` for MySQL but not for other databases.
> - In the same vein, is there a way to get a field name the same way
> Rail's
> ORM would put it in a SQL query?
> - Maybe there's a way to get both, the table name and the field name in
> one
> operation. I'm imaging something like Item.voteable_id.for_query
> => "`items`.`voteable`".
Check out quote_table_name and quote_column_name.
> - How do I escape code to avoid SQL injection when not in conditions?
> I'm
> using the user_id variable directly in a query and although it's
> impossible
> for a user to put anything in it, I'd rather escape it properly. In a
> condition I would do ['user_id = ?', user_id], but in a join or a
> select,
> how do I do it?
You really shouldn't be using user_id in a join as you're doing in the
query below -- it belongs in a WHERE clause. And at that point,
find_by_sql will handle it. Check the docs.
> - Does my use of class constants here make sense?
You're not using class constants.
> - Is there any chance at all of using the ORM and less string?
Yes. See my suggestions below.
> - Any other thing to do to it?
>
> The code is this one
>
> class Item < ActiveRecord::Base
> has_many :votes, :as => :voteable
>
> def self.ranking(user_id)
> Item.find(:all,
> # items.* for all the Item attributes, score being the sum of
> votes,
> user_vote is the vote of user_id (0 if no vote) and voter_id is just
> user_id
> for latter reference.
> :select => "items.*,
> IFNULL(sum(all_votes.value), 0) as score,
> user_votes.value as user_vote,
> \"#{user_id}\" as voter_id",
> # The first join gets all the votes for a single item (to be
> summed
> latter).
> # The second join gets the vote for a single user for a single
> item.
> :joins => ["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 = \"#{user_id}\" and
> user_votes.voteable_type = \"Item\""
> ],
> :group => :id,
> :order => "score DESC")
>
> # This is the query it should generate
> # SELECT items.*, user_votes.value as user_vote,
> sum(all_votes.value) as
> score
> # 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
> # ORDER BY score DESC
> end
I think your logic is poor. Although it's tempting, it feels wrong to
join both all the votes (in the first join) and a subset of votes (in
the second join). I usually advocate one big query, but in this case I
think it should probably be two queries:
Vote.sum(:value, :conditions => {:voteable_type => 'Item'}, :group =>
'voteable_id', :joins => 'left join items on (item.id = voteable_id)',
:order => 'sum(value) desc')
Item.find(:all, :joins => :votes, :conditions => {:votes => {:user_id =>
user_id}})
Best,
--
Marnen Laibow-Koser
http://www.marnen.org
[email protected]
--
Posted via http://www.ruby-forum.com/.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Ruby
on Rails: Talk" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---
