# How can I improve a Ruby on Rails code that hast a lot of SQL as strings?

**URL:** <https://www.ruby-forum.com/t/how-can-i-improve-a-ruby-on-rails-code-that-hast-a-lot-of-sql-as-strings/178658>\
**Category:** Rails\
**Created:** [November 9, 2009, 3:28am UTC](https://www.ruby-forum.com/t/how-can-i-improve-a-ruby-on-rails-code-that-hast-a-lot-of-sql-as-strings/178658 "2009-11-09T03:28:56Z")\
**Posts on this page:** 4\
**Page:** 1

<div class="post-metadata">

**Author:** ![JSPablo\_FernSSSSnd](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/e9bcb4/32.png) [@JSPablo\_FernSSSSnd](https://www.ruby-forum.com/u/JSPablo_FernSSSSnd)\
**Post date:** [November 9, 2009, 3:28am UTC](https://www.ruby-forum.com/t/how-can-i-improve-a-ruby-on-rails-code-that-hast-a-lot-of-sql-as-strings/178658/1 "2009-11-09T03:28:56Z")

</div>

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?

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`”.
- 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?
- Does my use of class constants here make sense?
- Is there any chance at all of using the ORM and less string?
- 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

def score  
s = read\_attribute(“score”)  
if s == nil  
votes.sum :value  
else  
Integer(s)  
end  
end

def user\_vote(user\_id)  
if Integer(read\_attribute(“voter\_id”)) == user\_id  
Integer(read\_attribute(“user\_vote”))  
else  
vote = votes.find(:first, :conditions =\> [“user\_id = ?”, user\_id])  
if vote  
vote.value  
else  
0  
end  
end  
end  
end

## Thanks.

J. Pablo Fernández [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) ([http://pupeno.com](http://pupeno.com))

---

<div class="post-metadata">

**Author:** ![JSPablo\_FernSSSSnd](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/e9bcb4/32.png) [@JSPablo\_FernSSSSnd](https://www.ruby-forum.com/u/JSPablo_FernSSSSnd)\
**Post date:** [November 9, 2009, 4:37am UTC](https://www.ruby-forum.com/t/how-can-i-improve-a-ruby-on-rails-code-that-hast-a-lot-of-sql-as-strings/178658/2 "2009-11-09T04:37:26Z")

</div>

check out the acts as voteable plugin on github, looks like you’ve  
already  
got your modeling done correctly to just drop it in.

-C  
2009/11/8 J. Pablo Fernández [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid)

---

<div class="post-metadata">

**Author:** ![JSPablo\_FernSSSSnd](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/e9bcb4/32.png) [@JSPablo\_FernSSSSnd](https://www.ruby-forum.com/u/JSPablo_FernSSSSnd)\
**Post date:** [November 9, 2009, 8:46am UTC](https://www.ruby-forum.com/t/how-can-i-improve-a-ruby-on-rails-code-that-hast-a-lot-of-sql-as-strings/178658/3 "2009-11-09T08:46:43Z")

</div>

Hello Chris,

I’ve checked. There was one little thing the act as votable couldn’t do;  
I  
can’t remember exactly what, maybe the negative votes or something like  
that. So I did my own.

But what I’m really after is improving the so many strings.

On Mon, Nov 9, 2009 at 04:36, Chris Drappier  
[removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid)wrote:

> > my taste it has too many strings and harcoded values. I’d like to improve it
> > 
> > - Maybe there’s a way to get both, the table name and the field name in
> > 
> > ## Thanks.
> > 
> > J. Pablo Fernández [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) ([http://pupeno.com](http://pupeno.com))
> 
> >

–  
J. Pablo Fernández [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) ([http://pupeno.com](http://pupeno.com))

---

<div class="post-metadata">

**Author:** ![JSPablo\_FernSSSSnd](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/e9bcb4/32.png) [@JSPablo\_FernSSSSnd](https://www.ruby-forum.com/u/JSPablo_FernSSSSnd)\
**Post date:** [November 9, 2009, 5:26am UTC](https://www.ruby-forum.com/t/how-can-i-improve-a-ruby-on-rails-code-that-hast-a-lot-of-sql-as-strings/178658/4 "2009-11-09T05:26:02Z")

</div>

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](http://www.marnen.org)  
removed\_email\_address@domain.invalid