Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following query. I know there's an easier way to do this without the sub queries, or at least optimizing how they're used. I've read about cross joins and using HAVING but cannot figure out how to make this better as I know it's pretty bad.

Basically what I'm trying to do is get the top 10 stores within a category. For this category it is category with an id of 9 as you can see in the where clause. I am categorizing each individual user based on the most occurring category of the products they have. And then reordering the results by most orders and getting the first 10 results.

EDIT:

c.id = 9, which should be c.id = '$categoryId' (edited) is representative of where a numeric value would be passed into the query to get the top stores specific to a category id.

SELECT u.id, 
       u.avatar, 
       u.storeName,
       (SELECT COUNT(o.id) FROM orders o WHERE o.storeId = u.id) AS orders
FROM users u
WHERE (SELECT c.id
       FROM categories c, products p
       WHERE c.id = p.categoryId
       AND p.userId = u.id
       GROUP BY p.categoryId
       ORDER BY COUNT(p.id) DESC
       LIMIT 1) = '$categoryId'
ORDER BY orders DESC
LIMIT 10
share|improve this question
    
Why not just use WHERE c.id = 9? Do you need to make sure it exists in the products table? –  ckuhn203 Jul 17 at 10:00
    
It only exists in the products table, and we want to get the most frequently occuring ID –  Braydon Batungbacal Jul 17 at 10:46
    
Please join me in chat I have some questions. –  ckuhn203 Jul 17 at 11:14
add comment

3 Answers 3

You likely don't want to use CROSS JOIN as the performance is terrible and the cartesian product it returns is rarely useful. Instead, use INNER JOIN, LEFT JOIN etc.


I'm not a big fan of the way you named your table variables, but in such a tiny query I guess it's OK. u, o, c, p that would get really confusing in a larger script. usr, ord, cat, prd would at least give Mr. Maintainer an idea of what the variable means.


This:

   FROM categories cat, products prd
   WHERE cat.id = prd.categoryId
   AND prd.userId = usr.id

That is an old-style JOIN and should not be used. Instead use this:

   FROM categories cat INNER JOIN products prd
   ON cat.id = prd.categoryId
   AND prd.userId = usr.id

So all this taken into account, here is how I would write your query:

SELECT usr.id, 
       usr.avatar, 
       usr.storeName,
       COUNT(ord.id) AS orders
FROM users usr
INNER JOIN orders ord ON ord.storeId = usr.id
WHERE (SELECT cat.id
       FROM categories cat, products prd
       WHERE cat.id = prd.categoryId
       AND prd.userId = usr.id
       GROUP BY prd.categoryId
       ORDER BY COUNT(prd.id) DESC
       LIMIT 1) = 9
ORDER BY COUNT(ord.id) DESC
LIMIT 10;
share|improve this answer
add comment

There are some things that you don't need here

SELECT u.id, 
       u.avatar, 
       u.storeName,
       (SELECT COUNT(o.id) FROM orders o WHERE o.storeId = u.id) AS orders
FROM users u
WHERE (SELECT c.id
       FROM categories c, products p
       WHERE c.id = p.categoryId
       AND p.userId = u.id
       GROUP BY p.categoryId
       ORDER BY COUNT(p.id) DESC
       LIMIT 1) = 9
ORDER BY orders DESC
LIMIT 10

like the ORDER BY and the LIMIT 1 and the GROUP BY in the Select statement that is nested in your where clause.

SELECT u.id, 
       u.avatar, 
       u.storeName,
       (SELECT COUNT(o.id) FROM orders o WHERE o.storeId = u.id) AS orders
FROM users u
WHERE (SELECT c.id
       FROM categories c, products p
       WHERE c.id = p.categoryId
       AND p.userId = u.id
       ) = 9
ORDER BY orders DESC
LIMIT 10

You should also rename your column orders to orderCount

one more thing that I would suggest is to take it all out of the WHERE clause, this is slow

do it like this

SELECT u.id, 
       u.avatar, 
       u.storeName,
       (SELECT COUNT(o.id) FROM orders o WHERE o.storeId = u.id) AS orderCount
FROM users u INNER JOIN products p ON u.id = p.userId 
WHERE p.categoryID = 9
ORDER BY orderCount DESC
LIMIT 10
share|improve this answer
    
It's not explicitly stated here, but it came out in chat that there should be no results if the category is not the top category for a user. –  ckuhn203 Jul 17 at 15:53
    
oops @ckuhn203 I left that out of my query. –  Malachi Jul 17 at 16:00
add comment

I add this as a separate answer because it doesn't follow the same path as my first answer.


You really don't need to grab the category table inside the sub query in the WHERE clause

it could look like this

SELECT u.id, 
       u.avatar, 
       u.storeName,
       (SELECT COUNT(o.id) FROM orders o WHERE o.storeId = u.id) AS orders
FROM users u
WHERE (SELECT p.categoryId
       FROM products p
       WHERE p.userId = u.id
       GROUP BY p.categoryId
       ORDER BY COUNT(p.id) DESC
       LIMIT 1) = 9
ORDER BY orders DESC
LIMIT 10

this would get rid of an extra table look-up that isn't needed inside of the WHERE, if you can try to avoid using a sub query nested inside of a WHERE clause because that query will be run on every row returned by the containing query, it's like using a UDF in the WHERE clause in SQL Server, it's horrible for performance.

I am sure you have a valid reason here, but I would definitely look into my other answer and see if it is going to give you the results you are looking for (which it should if I understand what you are doing correctly)

share|improve this answer
    
Ahh, right. That simplifies things a little. As for your previous answer, @ckuhn203 added in the comments, which I will edit my post to include, about the top category results being a factor. –  Braydon Batungbacal Jul 18 at 0:12
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.