4
\$\begingroup\$

I have the following MySQL query which selects product details and whether the viewing user likes them. I feel like it could be more efficient and that there is a more practical approach than using a sub-query here.

Any suggestions?

SELECT p.*, 
         pm.filename AS thumbnail,
         (SELECT COUNT(id) FROM likes WHERE productId = p.id AND 
                userId = '$viewingUserId' AND isActive = '1' LIMIT 1) 
            as liked
            FROM products p, productmedia pm
        WHERE p.userId = '$userId'
        AND p.id = pm.productId
        AND pm.sortOrder = '0'
        ORDER BY p.timestamp DESC
\$\endgroup\$
  • 1
    \$\begingroup\$ To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your query does and the schema of your database table(s), the easier it will be for reviewers to help you. See also this meta question \$\endgroup\$ – Simon Forsberg Jun 25 '14 at 10:53
  • 1
    \$\begingroup\$ Does this query actually do what it's supposed to do? It looks to me like it will only select products the user has favorited. \$\endgroup\$ – cimmanon Jul 28 '14 at 1:41
  • \$\begingroup\$ pm.sortOrder = 0 seems like an odd search criterion to me. What is that trying to accomplish? \$\endgroup\$ – 200_success Jul 29 '14 at 2:10
  • \$\begingroup\$ pm contains multiple images that could be related to 1 product. A product image with a sort order of 0 (The first image for the product) is the cover photo that we want to pull in this query \$\endgroup\$ – Braydon Batungbacal Jul 29 '14 at 4:12
15
\$\begingroup\$

More important than the performance, your code is very likely vulnerable to SQL Injection. (I can't say for sure without looking at your PHP code)

If $viewingUserId or $userId is chosen by the user in any way, then you have a problem. And even if it is not, I would highly recommend using prepared queries.

I don't know if you're using the mysql_* methods or mysqli_* methods or the PDO extention, but you should be aware that the mysql_* methods are deprecated (That means: Don't use them) and that you should either use mysqli (note the i) or PDO.

\$\endgroup\$
  • \$\begingroup\$ All values coming in via $_POST, $_GET are filtered prior to going through any of the controllers. \$\endgroup\$ – Braydon Batungbacal Jun 25 '14 at 20:20
  • \$\begingroup\$ I've been meaning to switch to prepared queries actually, haven't gotten to it yet. \$\endgroup\$ – Braydon Batungbacal Jun 25 '14 at 20:20
  • \$\begingroup\$ @BraydonBatungbacal google.com.br/… this is good reading as some other guys suggested me... \$\endgroup\$ – matheussilvapb Jul 26 '14 at 0:07
  • \$\begingroup\$ Hey guys, I rewrote my database layer for mysqli, all is well now :) Thank you \$\endgroup\$ – Braydon Batungbacal Jul 26 '14 at 1:08
7
\$\begingroup\$

Naming

You should name your aliases in such a way that looking at them tells you something about their meaning. p and pm tell nothing to the next programmer who will have to maintain your code. It's better to type a few extra letters and add clarity, like prod and prMedia.

Using SELECT *

SELECT * is not very efficient as SQL has to go back and look up every column's DDL in your information schema. Only SELECT the columns you actually need for faster query.

Old-style JOIN

You have this:

FROM products p, productmedia pm
        WHERE p.userId = '$userId'
        AND p.id = pm.productId
        AND pm.sortOrder = '0'

This is bad practice and will likely become deprecated eventually. Instead, use an explicit type of join. I will use INNER JOIN in this case.

Numbers stored as text?

I saw isActive = '1' and prMedia.sortOrder = '0'. It's better to use INT for integers and BOOLEAN for true/false fields.

The modified script

I eliminated your subquery by working it into the WHERE clause.

SELECT 
    prod.*, 
    prMedia.filename AS thumbnail,
    COUNT(prod.id) AS liked
FROM products AS prod
  INNER JOIN productmedia AS prMedia
    ON prod.userId = '$userId'
    AND prod.id = prMedia.productId
WHERE prod.userId = '$viewingUserId' 
    AND prMedia.sortOrder = '0'
    AND prod.isActive = '1' 
ORDER BY prod.timestamp DESC
LIMIT 1;
\$\endgroup\$
  • 1
    \$\begingroup\$ Also, do what @SimonAndréForsberg said. \$\endgroup\$ – Phrancis Jul 25 '14 at 21:48
  • \$\begingroup\$ Why stop at prMedia? Why not just type out productMedia? That way, there's absolutely no confusion. \$\endgroup\$ – cimmanon Jul 27 '14 at 16:46
1
\$\begingroup\$

You're going to have to forgive me here if I get the syntax not quite right. I primarily work with PostgreSQL, not MySQL.

Unless the user is allowed to like an item more than once, there's no reason to perform a count: the user has either liked the item or they haven't. If there are no restrictions on how often the user is allowed to favorite in the database, there should be (judging from your query, you'd be needing a partial unique index in a PostgreSQL database to ensure there's only 1 active like per user/product).

To get the simplest query, you'll want to use a LEFT JOIN rather than a subquery. This query assumes that you want to select items the user hasn't favorited (which your current query doesn't appear to do):

SELECT
    products.*, 
    productmedia.filename AS thumbnail,
    CASE WHEN likes.productId IS NOT NULL THEN true ELSE false END AS liked
FROM
    products
    JOIN productmedia ON productmedia.productId = products.id
    LEFT JOIN likes ON likes.productId = products.id AND userId = ? AND isActive = 1
WHERE
    productmedia.sortOrder = '0'
ORDER BY
    products.timestamp DESC;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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