As a vqMod developer, one of the more annoying things in OpenCart are the SQL statements, for the following reasons:
- They're almost always on one long line
- The DB_PREFIX is messy and makes the SQL harder to read
- Instead of assigning and passing PHP variables to SQL, they are called inside the SQL statement themselves.
catalog\model\catalog\product.php
Code: Select all
$query = $this->db->query("SELECT DISTINCT *, pd.name AS name, p.image, m.name AS manufacturer, (SELECT price FROM " . DB_PREFIX . "product_discount pd2 WHERE pd2.product_id = p.product_id AND pd2.customer_group_id = '" . (int)$customer_group_id . "' AND pd2.quantity = '1' AND ((pd2.date_start = '0000-00-00' OR pd2.date_start < NOW()) AND (pd2.date_end = '0000-00-00' OR pd2.date_end > NOW())) ORDER BY pd2.priority ASC, pd2.price ASC LIMIT 1) AS discount, (SELECT price FROM " . DB_PREFIX . "product_special ps WHERE ps.product_id = p.product_id AND ps.customer_group_id = '" . (int)$customer_group_id . "' AND ((ps.date_start = '0000-00-00' OR ps.date_start < NOW()) AND (ps.date_end = '0000-00-00' OR ps.date_end > NOW())) ORDER BY ps.priority ASC, ps.price ASC LIMIT 1) AS special, (SELECT points FROM " . DB_PREFIX . "product_reward pr WHERE pr.product_id = p.product_id AND customer_group_id = '" . (int)$customer_group_id . "') AS reward, (SELECT ss.name FROM " . DB_PREFIX . "stock_status ss WHERE ss.stock_status_id = p.stock_status_id AND ss.language_id = '" . (int)$this->config->get('config_language_id') . "') AS stock_status, (SELECT wcd.unit FROM " . DB_PREFIX . "weight_class_description wcd WHERE p.weight_class_id = wcd.weight_class_id AND wcd.language_id = '" . (int)$this->config->get('config_language_id') . "') AS weight_class, (SELECT lcd.unit FROM " . DB_PREFIX . "length_class_description lcd WHERE p.length_class_id = lcd.length_class_id AND lcd.language_id = '" . (int)$this->config->get('config_language_id') . "') AS length_class, (SELECT AVG(rating) AS total FROM " . DB_PREFIX . "review r1 WHERE r1.product_id = p.product_id AND r1.status = '1' GROUP BY r1.product_id) AS rating, (SELECT COUNT(*) AS total FROM " . DB_PREFIX . "review r2 WHERE r2.product_id = p.product_id AND r2.status = '1' GROUP BY r2.product_id) AS reviews, p.sort_order FROM " . DB_PREFIX . "product p LEFT JOIN " . DB_PREFIX . "product_description pd ON (p.product_id = pd.product_id) LEFT JOIN " . DB_PREFIX . "product_to_store p2s ON (p.product_id = p2s.product_id) LEFT JOIN " . DB_PREFIX . "manufacturer m ON (p.manufacturer_id = m.manufacturer_id) WHERE p.product_id = '" . (int)$product_id . "' AND pd.language_id = '" . (int)$this->config->get('config_language_id') . "' AND p.status = '1' AND p.date_available <= NOW() AND p2s.store_id = '" . (int)$this->config->get('config_store_id') . "'");This is incredibly difficult to follow. It's not impossible, but it is difficult. It makes optimizing and improving the code harder, and could discourage developers from working on the project.
What follows below are my own personal suggestions on how this could be improved. It's not about pushing my own particular programming style onto everyone else, it's just to get the conversation going about how we could make this better.
I'd start by assigning the query to a variable before sending it to the database.
Code: Select all
$sql = "SELECT DISTINCT *, pd.name AS name, p.image, m.name AS manufacturer, (SELECT price FROM " . DB_PREFIX . "product_discount pd2 WHERE pd2.product_id = p.product_id AND pd2.customer_group_id = '" . (int)$customer_group_id . "' AND pd2.quantity = '1' AND ((pd2.date_start = '0000-00-00' OR pd2.date_start < NOW()) AND (pd2.date_end = '0000-00-00' OR pd2.date_end > NOW())) ORDER BY pd2.priority ASC, pd2.price ASC LIMIT 1) AS discount, (SELECT price FROM " . DB_PREFIX . "product_special ps WHERE ps.product_id = p.product_id AND ps.customer_group_id = '" . (int)$customer_group_id . "' AND ((ps.date_start = '0000-00-00' OR ps.date_start < NOW()) AND (ps.date_end = '0000-00-00' OR ps.date_end > NOW())) ORDER BY ps.priority ASC, ps.price ASC LIMIT 1) AS special, (SELECT points FROM " . DB_PREFIX . "product_reward pr WHERE pr.product_id = p.product_id AND customer_group_id = '" . (int)$customer_group_id . "') AS reward, (SELECT ss.name FROM " . DB_PREFIX . "stock_status ss WHERE ss.stock_status_id = p.stock_status_id AND ss.language_id = '" . (int)$this->config->get('config_language_id') . "') AS stock_status, (SELECT wcd.unit FROM " . DB_PREFIX . "weight_class_description wcd WHERE p.weight_class_id = wcd.weight_class_id AND wcd.language_id = '" . (int)$this->config->get('config_language_id') . "') AS weight_class, (SELECT lcd.unit FROM " . DB_PREFIX . "length_class_description lcd WHERE p.length_class_id = lcd.length_class_id AND lcd.language_id = '" . (int)$this->config->get('config_language_id') . "') AS length_class, (SELECT AVG(rating) AS total FROM " . DB_PREFIX . "review r1 WHERE r1.product_id = p.product_id AND r1.status = '1' GROUP BY r1.product_id) AS rating, (SELECT COUNT(*) AS total FROM " . DB_PREFIX . "review r2 WHERE r2.product_id = p.product_id AND r2.status = '1' GROUP BY r2.product_id) AS reviews, p.sort_order FROM " . DB_PREFIX . "product p LEFT JOIN " . DB_PREFIX . "product_description pd ON (p.product_id = pd.product_id) LEFT JOIN " . DB_PREFIX . "product_to_store p2s ON (p.product_id = p2s.product_id) LEFT JOIN " . DB_PREFIX . "manufacturer m ON (p.manufacturer_id = m.manufacturer_id) WHERE p.product_id = '" . (int)$product_id . "' AND pd.language_id = '" . (int)$this->config->get('config_language_id') . "' AND p.status = '1' AND p.date_available <= NOW() AND p2s.store_id = '" . (int)$this->config->get('config_store_id') . "'";
$query = $this->db->query($sql);
SQL Before:
Code: Select all
"... LEFT JOIN " . DB_PREFIX . "manufacturer m ON ..."Code: Select all
"... LEFT JOIN {oc}manufacturer m ON..."Code: Select all
public function query($sql) {
$sql = str_replace("{oc}",DB_PREFIX,$sql); // Added: Replace DB prefix before query execution
return $this->driver->query($sql);
}
By doing this, the above SQL statement becomes:
Code: Select all
$sql = "SELECT DISTINCT *, pd.name AS name, p.image, m.name AS manufacturer, (SELECT price FROM {oc}product_discount pd2 WHERE pd2.product_id = p.product_id AND pd2.customer_group_id = '" . (int)$customer_group_id . "' AND pd2.quantity = '1' AND ((pd2.date_start = '0000-00-00' OR pd2.date_start < NOW()) AND (pd2.date_end = '0000-00-00' OR pd2.date_end > NOW())) ORDER BY pd2.priority ASC, pd2.price ASC LIMIT 1) AS discount, (SELECT price FROM {oc}product_special ps WHERE ps.product_id = p.product_id AND ps.customer_group_id = '" . (int)$customer_group_id . "' AND ((ps.date_start = '0000-00-00' OR ps.date_start < NOW()) AND (ps.date_end = '0000-00-00' OR ps.date_end > NOW())) ORDER BY ps.priority ASC, ps.price ASC LIMIT 1) AS special, (SELECT points FROM {oc}product_reward pr WHERE pr.product_id = p.product_id AND customer_group_id = '" . (int)$customer_group_id . "') AS reward, (SELECT ss.name FROM {oc}stock_status ss WHERE ss.stock_status_id = p.stock_status_id AND ss.language_id = '" . (int)$this->config->get('config_language_id') . "') AS stock_status, (SELECT wcd.unit FROM {oc}weight_class_description wcd WHERE p.weight_class_id = wcd.weight_class_id AND wcd.language_id = '" . (int)$this->config->get('config_language_id') . "') AS weight_class, (SELECT lcd.unit FROM {oc}length_class_description lcd WHERE p.length_class_id = lcd.length_class_id AND lcd.language_id = '" . (int)$this->config->get('config_language_id') . "') AS length_class, (SELECT AVG(rating) AS total FROM {oc}review r1 WHERE r1.product_id = p.product_id AND r1.status = '1' GROUP BY r1.product_id) AS rating, (SELECT COUNT(*) AS total FROM {oc}review r2 WHERE r2.product_id = p.product_id AND r2.status = '1' GROUP BY r2.product_id) AS reviews, p.sort_order FROM {oc}product p LEFT JOIN {oc}product_description pd ON (p.product_id = pd.product_id) LEFT JOIN {oc}product_to_store p2s ON (p.product_id = p2s.product_id) LEFT JOIN {oc}manufacturer m ON (p.manufacturer_id = m.manufacturer_id) WHERE p.product_id = '" . (int)$product_id . "' AND pd.language_id = '" . (int)$this->config->get('config_language_id') . "' AND p.status = '1' AND p.date_available <= NOW() AND p2s.store_id = '" . (int)$this->config->get('config_store_id') . "'";
We can also assign config values to variables:
Code: Select all
$language_id = (int)$this->config->get('config_language_id');
$store_id = (int)$this->config->get('config_store_id');
$group_id = (int)$customer_group_id;
$sql = "SELECT DISTINCT *, pd.name AS name, p.image, m.name AS manufacturer, (SELECT price FROM {oc}product_discount pd2 WHERE pd2.product_id = p.product_id AND pd2.customer_group_id = ".$group_id." AND pd2.quantity = '1' AND ((pd2.date_start = '0000-00-00' OR pd2.date_start < NOW()) AND (pd2.date_end = '0000-00-00' OR pd2.date_end > NOW())) ORDER BY pd2.priority ASC, pd2.price ASC LIMIT 1) AS discount, (SELECT price FROM {oc}product_special ps WHERE ps.product_id = p.product_id AND ps.customer_group_id = ".$group_id." AND ((ps.date_start = '0000-00-00' OR ps.date_start < NOW()) AND (ps.date_end = '0000-00-00' OR ps.date_end > NOW())) ORDER BY ps.priority ASC, ps.price ASC LIMIT 1) AS special, (SELECT points FROM {oc}product_reward pr WHERE pr.product_id = p.product_id AND customer_group_id = ".$group_id.") AS reward, (SELECT ss.name FROM {oc}stock_status ss WHERE ss.stock_status_id = p.stock_status_id AND ss.language_id = ".$language_id.") AS stock_status, (SELECT wcd.unit FROM {oc}weight_class_description wcd WHERE p.weight_class_id = wcd.weight_class_id AND wcd.language_id = ".$language_id.") AS weight_class, (SELECT lcd.unit FROM {oc}length_class_description lcd WHERE p.length_class_id = lcd.length_class_id AND lcd.language_id = ".$language_id.") AS length_class, (SELECT AVG(rating) AS total FROM {oc}review r1 WHERE r1.product_id = p.product_id AND r1.status = '1' GROUP BY r1.product_id) AS rating, (SELECT COUNT(*) AS total FROM {oc}review r2 WHERE r2.product_id = p.product_id AND r2.status = '1' GROUP BY r2.product_id) AS reviews, p.sort_order FROM {oc}product p LEFT JOIN {oc}product_description pd ON (p.product_id = pd.product_id) LEFT JOIN {oc}product_to_store p2s ON (p.product_id = p2s.product_id) LEFT JOIN {oc}manufacturer m ON (p.manufacturer_id = m.manufacturer_id) WHERE p.product_id = '" . (int)$product_id . "' AND pd.language_id = ".$language_id." AND p.status = '1' AND p.date_available <= NOW() AND p2s.store_id = ".$store_id."";
$query = $this->db->query($sql);
Finally, to make the queries easier to read, I'd look at splitting the SQL over multiple lines.
Code: Select all
$language_id = (int)$this->config->get('config_language_id');
$store_id = (int)$this->config->get('config_store_id');
$group_id = (int)$customer_group_id;
$sql = "SELECT DISTINCT *, pd.name AS name, p.image, m.name AS manufacturer,
(SELECT price FROM {oc}product_discount pd2 WHERE pd2.product_id = p.product_id AND pd2.customer_group_id = ".$group_id." AND pd2.quantity = '1' AND ((pd2.date_start = '0000-00-00' OR pd2.date_start < NOW()) AND (pd2.date_end = '0000-00-00' OR pd2.date_end > NOW())) ORDER BY pd2.priority ASC, pd2.price ASC LIMIT 1) AS discount,
(SELECT price FROM {oc}product_special ps WHERE ps.product_id = p.product_id AND ps.customer_group_id = ".$group_id." AND ((ps.date_start = '0000-00-00' OR ps.date_start < NOW()) AND (ps.date_end = '0000-00-00' OR ps.date_end > NOW())) ORDER BY ps.priority ASC, ps.price ASC LIMIT 1) AS special,
(SELECT points FROM {oc}product_reward pr WHERE pr.product_id = p.product_id AND customer_group_id = ".$group_id.") AS reward,
(SELECT ss.name FROM {oc}stock_status ss WHERE ss.stock_status_id = p.stock_status_id AND ss.language_id = ".$language_id.") AS stock_status,
(SELECT wcd.unit FROM {oc}weight_class_description wcd WHERE p.weight_class_id = wcd.weight_class_id AND wcd.language_id = ".$language_id.") AS weight_class,
(SELECT lcd.unit FROM {oc}length_class_description lcd WHERE p.length_class_id = lcd.length_class_id AND lcd.language_id = ".$language_id.") AS length_class,
(SELECT AVG(rating) AS total FROM {oc}review r1 WHERE r1.product_id = p.product_id AND r1.status = '1' GROUP BY r1.product_id) AS rating,
(SELECT COUNT(*) AS total FROM {oc}review r2 WHERE r2.product_id = p.product_id AND r2.status = '1' GROUP BY r2.product_id) AS reviews,
p.sort_order
FROM {oc}product p
LEFT JOIN {oc}product_description pd ON (p.product_id = pd.product_id)
LEFT JOIN {oc}product_to_store p2s ON (p.product_id = p2s.product_id)
LEFT JOIN {oc}manufacturer m ON (p.manufacturer_id = m.manufacturer_id)
WHERE p.product_id = '" . (int)$product_id . "'
AND pd.language_id = ".$language_id."
AND p.status = '1'
AND p.date_available <= NOW()
AND p2s.store_id = ".$store_id."
LIMIT 1
";
$query = $this->db->query($sql);
The rest of the SQL isn't too bad, I added a LIMIT 1 (because we're only ever selecting 1 product, there's no need to continue scanning the table once this product has been found).
Some of those subqueries could also be rewritten as LEFT JOINs, but in order to do that we'd need to make some structural changes (a lot more work).
As I said, these are just my own suggestions to try to improve the code even further. It's already looking much better in the repository, the project is heading in the right direction and most of our client stores are running very fast. We'd like to be able to contribute more functionality over the next year to help get it to the next level.
