Is this SQL select code following good practice?

Question

I am using SQLite and will port to MySQL (5) later.

I wanted to know if I am doing something I shouldn't be doing. I tried purposely to design so I'll compare to 0 instead of 1 (I changed hasApproved to NotApproved to do this, not a big deal and I haven't written any code). I was told I never need to write a subquery but I do here. My Votes table is just id, ip, postid (I don't think I can write that subquery as a join instead?) and that's pretty much all that is on my mind.

Naming conventions I don't really care about since the tables are created via reflection and is all over the place.

select 
    id, 
    name, 
    body, 
    upvotes, 
    downvotes, 
    (select 1 from UpVotes where IPAddr=? AND post=Post.id) as myup, 
    (select 1 from DownVotes where IPAddr=@0 AND post=Post.id) as mydown
from Post 
where 
    flag = '0'
limit ?, ?"

Answer 1

You can use joins to achieve the same thing, and I would expect joins to work a lot more efficiently than embeded selects.

Answer 2

I'm guessing that you're trying to ensure that a user only votes once on each post here.

I wouldn't - I don't - use separate tables for up votes and down votes. Add vote type to your votes table and you won't need correlated subqueries.

Answer 3

Here is my opinions:

1. It seems that table "UpVotes" and "DownVotes" have same structure and can be merged into one table.

2. The relation between table "Post" and "Up/DownVotes" can be constrained by foreign key.

3. Although I am not sure about the performance difference, but I think it would be better to use "join" mechanism rather than nesting two select statement in a select statement.

Answer 4

Since you're asking about good practices... the "upvotes" and "downvotes" appearing in your Posts table looks like you're duplicating data in your database. That's a problem, because now you always have to worry whether or not the data is in sync and correct. If you want to know the number of upvotes then count them, don't also store them in the Post table. I'm not positive that is what you're doing, but it's a guess.

Onto your query... You will probably get better performance using a JOINed subquery instead of how you have it. With the scalar subqueries as columns they have to be run once for every row that is returned. That could be a pretty big performance hit if you're returning a bunch of rows. Instead, try:

SELECT
    P.id,
    P.name,
    P.body,
    P.upvotes,
    P.downvotes,
    COALESCE(UV.cnt, 0) AS upvotes2,
    COALESCE(DV.cnt, 0) AS downvotes2
FROM
    dbo.Posts P
LEFT OUTER JOIN (SELECT post_id, COUNT(*) cnt FROM dbo.UpVotes GROUP BY post_id) AS UV ON UV.post_id = P.id
LEFT OUTER JOIN (SELECT post_id, COUNT(*) cnt FROM dbo.DownVotes GROUP BY post_id) AS DV ON DV.post_id = P.id

Compare it to your own query and see if it gives you better performance.

EDIT: A couple of other posters have advocated a single table for up/down votes. They are absolutely correct. That makes the query even easier and also probably much faster:

SELECT
    P.id,
    P.name,
    P.body,
    P.upvotes,
    P.downvotes,
    SUM(CASE WHEN V.vote_type = 'UP' THEN 1 ELSE 0 END) AS upvotes2,
    SUM(CASE WHEN V.vote_type = 'DOWN' THEN 1 ELSE 0 END) AS downvotes2,
FROM
    dbo.Posts P
LEFT OUTER JOIN Votes V ON
    V.post_id = P.id
GROUP BY
    P.id,
    P.name,
    P.body,
    P.upvotes,
    P.downvotes