2
\$\begingroup\$

I'm making a simple webshop, it's not the full code, there are more security procedures. I'm just want to know your opinions. Thank you.

These SQL tables I have:

(I have more column in this table this is just an example)

customers:

id name phone address zip city country o_date
1 Test 12345 Test Adress 1234 Test City Test Country 01-02-2022

orders

customerid orderid orderdate total_price
1 OS1 01-02-2022 19

orderitems

id orderid productid item_price
1 OS1 P1 19

shipping

orderid shipping_method shipping_price tracking_id status sent_date
OS1 courier 0 12345 sent 02-01-2022

products

product_id productprice currency productqty productname weight
P1 19 19 USD 1 Test Product

You must have noticed that I have 3 times the price of the order/product in the tables. The reason why is in the products table I have the current product price this can be changed, and in the another tables I have the product price which was at the time of ordering.

After submitting the form the customer will land on this page:

order.php

/// Insert into customers table
    $data = [
        'name' => $name,
        'phone' => $phone,
        'adress' => $adress,
        'zip' => $zip,
        'city' => $city,
        'country' => $country,
        'o_date' => $date
    ];
    $sqlinsertcustomer = "INSERT INTO customers (id, name, phone, adress, zip, city, country, o_date ) VALUES ( '', :name, :phone, :adress, :zip, :city, :country, :o_date)";
    $stmt = $conn->prepare($sqlinsertcustomer);
    $stmt->execute($data);
    $last_id = $conn->lastInsertId();
    $order_id = "OS". $last_id;
    
    /// Get the total price from the actual product. 
    
    $stmtpprice = $conn->prepare("SELECT productprice FROM products WHERE product_id = :product_id;");
    $stmtpprice->execute([":product_id"=>$productid]);
    $productprice = $stmtsms->fetch();
    $totalpprice = $productprice["productprice"];
    
/// Insert into Orders, Orderitems, Shipping Table
    $stmtorders = $conn->prepare("INSERT INTO orders (customerid, orderid, orderdate, total_price) VALUES (:customerid, :orderid, :orderdate, :total_price);");
    $stmtorderitems = $conn->prepare("INSERT INTO orderitems (id, orderid, productid, item_price) VALUES ('', :orderid, :productid, :itemprice);");
    $stmtshipping = $conn->prepare("INSERT INTO shipping (orderid, shipping_method, shipping_price, tracking_id, status, sent_date) VALUES (:orderid, :shipping_method, :shipping_price, '0', '0', :sent_date);");
    $stmtorders->execute([":customerid"=>$last_id,
                          ":orderid"=>$order_id,
                          ":orderdate"=>$date],
                          ":totalprice"=>$totalpprice]);
    $stmtorderitems->execute([":orderid"=>$order_id,
                              ":productid"=>$productid],
                              ":itemprice"=>$totalpprice]);
    $stmtshipping->execute([":orderid"=>$order_id,
                            ":shipping_method"=>$shippingmethod,
                            ":shipping_price"=>$shippingprice,
                            ":sent_date"=>$date]);

What do you thing about processing and about the table structure? Thank you!

\$\endgroup\$
1
  • \$\begingroup\$ In most of the tables you are consistent. The column names are unique. Why two columns "id" and not "item_id" and "cust_id". It is more of being consistent than anything else. I have found unique column names being easier to manage. \$\endgroup\$
    – sibert
    Feb 1, 2022 at 9:05

2 Answers 2

4
\$\begingroup\$

Some minor suggestions:

  • I would move the code that retrieves the current product price to a dedicated function.
  • The main code should be a standalone function too
  • You should use a transaction to wrap up the whole sequence of operations, so that in case of error, you don't end up with inconsistent data and orphaned records.
  • Separate the three blocks that write to tables Orders, Orderitems, Shipping Table.
  • Add spacing where appropriate eg: $stmtorderitems->execute([":orderid" => $order_id,...
  • Improve naming a little bit, for example $last_id should be $customer_id so that there is no misunderstanding.
  • One potentially misleading statement: $totalpprice is the product price and not the total amount of the order as one might think. So just call it $product_price and there is no ambiguity.

$productprice = $stmtsms->fetch();
$totalpprice = $productprice["productprice"];
  • The order ID is generated like this: $order_id = "OS". $last_id;. Why not simply use an ID as well ? A client may have more than one order...
  • One problem in your code is that it requires horizontal scrolling, the SQL statement lines are too long. Wrap them up to have a better overview. I would perhaps do like this:

$stmtorders = $conn->prepare(
    "INSERT INTO orders (customerid, orderid, orderdate, total_price) 
    VALUES (:customerid, :orderid, :orderdate, :total_price);"
);

But you have considerable freedom here.

\$\endgroup\$
2
\$\begingroup\$

Quick remarks:

  • Your naming should be more consistent. You sometimes use snake_case (item_price) and elsewhere you simply compound words (productid). (I'd prefer PascalCase, but perhaps in your DB snake_case is more common. I'm not a fan of simply compounding words, it makes things unclear.)

  • Use descriptive column names, I have no idea what o_date is supposed to be.

  • No need to repeat information: item_price should just be price, since each entry is an item (in an order). Same with product_id and productprice in the table products.

  • I'm a fan of having an ID in each table, even if each entry could be identified by a combination of columns. To me a combination of columns feels less like an ID, and more like a constraint. (But that's an opinion, others will possibly/likely disagree.)

  • Don't needlessly abbreviate: you gain nothing by using qty instead of quantity.

\$\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.