4
\$\begingroup\$

Project description

I'm making a website listing products. The listing goes as following (See here):

  1. Product name
  2. Shop name
  3. Price in that shop
  4. If the product appears multiple times in the table for the same user (Which means he/she selected it in multiple stores. Merge them and just show multiple shop names)

Examples:

  • Product selected in 1 Shop: 2
  • Product selected in 2 Shops: 3
  • Product selected in 3 Shops: 4
  • and so on

PS: the names are always the same in the screenshots but they could be completely different ones

Database Tables:

Table user_interests:

user_hash           | shop_id    | product_id
--------------------+------------+---------------
a0a080f42e6f13b3... | 626        | 547261 
a0a080f42e6f13b3... | 554        | 283678 
a0a080f42e6f13b3... | 554        | 8680
a0a080f42e6f13b3... | 732        | 283678 
0aa1883c6411f787... | 865        | 283678 

Table shop_id:

shop_id    | shop_name
-----------+---------------
604        | Halle
626        | Vilvoorde
605        | Heverlee

Table product_price:

shop_id  | product_id | unit_price | ...
---------+------------+-------------------
626      | 547261     | 3,23        | ...
554      | 283678     | 4,59        | ...
732      | 283678     | 3,50        | ...
626      | 732        | 4,32        | ...
626      | 865        | 3,64        | ...

Table product_extra_info:

which contains extra info's

Encountered Issue

I am able to get a result (the one you're able to see on the screenshots) and it is displayed like I want it to be. Only problem is the code is really messy and filled with (I think) unnecessary sql requests. My comments are below the code

// Merge them and count how many 
$checkIfExistsMultipleSql = "SELECT `product_id`, COUNT(*)
FROM `user_interests` 
WHERE `user_hash`='$user_hash'
GROUP BY `product_id`
HAVING COUNT(*) >= 1;
";
$checkIfExistsMultipleResult = $mysqli->query($checkIfExistsMultipleSql);


// If not empty continue else tell user
if ($checkIfExistsMultipleResult) {
  // Make a website element for each product
  while($checkrow = $checkIfExistsMultipleResult ->fetch_assoc()) {

      $product = $checkrow['product_id'];
      

      -------------------[1]-------------------

      $multipleShopsSql = "SELECT `shop_id`
      FROM `user_interests` 
      WHERE `product_id`='$product' 
      AND `user_hash`='$user_hash'";
      $multipleShopsResult = $mysqli->query($multipleShopsSql);
      
      -------------------[2]-------------------

      $productInfoSql = "SELECT `name`, `picture`, `mesurement`, `content` 
      FROM `products_extra_info` 
      WHERE `product_id`='$product'";
      $productInfoResult = $mysqli->query($productInfoSql);
      $productInfo = $productInfoResult->fetch_assoc();
  
  1. I really do not like that part, I would like to "include" this one in the top one. (So what I'm trying to get is one select command which gives me a result showing this [product_id] is requested in [shop_id_1] and [shop_id_2] ...)
  2. For this one I would like to know what's better, as you can see I'm running this one, once for each product card. Is it good like that or should I run one big request and extract the data I need ?
\$\endgroup\$
10
  • 1
    \$\begingroup\$ Given that this is a sql review, you should post the definitions of the tables (and indexes), rather than just ASCII-art table drawings. \$\endgroup\$ Aug 25, 2022 at 14:31
  • \$\begingroup\$ Expect reviewers to pick up on the SQL-injection weakness, in any case. \$\endgroup\$ Aug 25, 2022 at 14:32
  • \$\begingroup\$ Where does $user_hash come from? \$\endgroup\$ Aug 25, 2022 at 15:54
  • \$\begingroup\$ Why does user_interests contain shop_id? Shouldn't it be taken from the product_price? \$\endgroup\$ Aug 25, 2022 at 16:17
  • 1
    \$\begingroup\$ "because all stores do not have the same promotions and prices" and so what? The code below does show all those "promotions and prices" without this useless field. If you want to limit a user to certain shops, it must be a different table. That links a user with a shop \$\endgroup\$ Aug 26, 2022 at 5:21

3 Answers 3

5
\$\begingroup\$

To elaborate on the Pslice's suggestion. Definitely, it must be just a single query with joins. Based on a slightly more sensible table structure, it could be like

select p.id, p.name product, pp.price, s.name shop from user_interests ui
join products p on p.id=ui.product_id
join product_prices pp on pp.product_id=ui.product_id
join shops s on s.id=pp.shop_id
where ui.user_id=1
order by p.id, price

And then, using PDO, we can already group the returned data by the product into a 3-dimensional array

$data = $pdo->query($sql)->fetchAll(PDO::FETCH_GROUP|PDO::FETCH_ASSOC);

that can be iterated over like this:

foreach ($data as $product) {
    echo $product[0]['product'],"\n";
    foreach ($product as $row) {
        echo "\t$row[price] $row[shop]\n"; 
    }
}

See it in action

\$\endgroup\$
5
\$\begingroup\$

Table Name

Is the table containing shop info really named shop_id, or was that a typo during creation of this post? If that is the actual name it seems shop would be more appropriate.

SQL Injection

It is surprising Your Common Sense didn't mention SQL Injection attacks, especially since they have a section about it in their page about prepared statements with PDO. It is unclear where $user_hash comes from but if there is a chance that the user could modify the value (e.g. in GET or POST data, a cookie, etc.) then the code could be susceptible to SQL Injection attacks. Even if the user has no way of modifying the value it is still a good habit to use that technique. See also answers to How can I prevent SQL injection in PHP?

Readability

While the code supplied is brief (and seems to be somewhat abridged) it is not too difficult to read. At the same time it is wise to follow common conventions. Many conventions are outlined in the PHP Standards Recommendations followed by many PHP developers and PSR-12 section 5 covers Control Structures covers Control Structures:

The general style rules for control structures are as follows:

  • There MUST be one space after the control structure keyword
  • There MUST NOT be a space after the opening parenthesis
  • There MUST NOT be a space before the closing parenthesis
  • There MUST be one space between the closing parenthesis and the opening brace
  • The structure body MUST be indented once
  • The body MUST be on the next line after the opening brace
  • The closing brace MUST be on the next line after the body

The body of each structure MUST be enclosed by braces. This standardizes how the structures look and reduces the likelihood of introducing errors as new lines get added to the body.

The if statement has appropriate spacing:

if ($checkIfExistsMultipleResult) {

However the while could benefit from a space before the opening parenthesis. And the space after $checkIfExistsMultipleResult seems a bit unorthodox.

while($checkrow = $checkIfExistsMultipleResult ->fetch_assoc()) {
     ^
\$\endgroup\$
1
  • \$\begingroup\$ I completely agree the code is really not that great. I have been struggling on that one for some days and made multiple test files before posting something. This is probably why I haven't been really careful while copy pasting and adding lines. But I will surely take note of that, thanks! \$\endgroup\$
    – Alpha.Ars
    Aug 25, 2022 at 22:41
2
\$\begingroup\$

Using INNER JOIN could consolidate the two queries using the product ID.

Other than that, I would recommend using PDO instead of MySQLi -- PHP delusions has the best definitive guide on using PHP with MySQL and I recommend following their implementation.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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