Efficiently accessing the database

Question

I followed this tutorial to create an SQL "factory".

The link shows how to make a class who's methods will output SQL statements based on the arguments you pass to it, and I've expanded on it with the class below.

Basically, as an example of its usage: if you wanted to select from a database you can pass it an array of attributes to select, followed by the table name, and if there are conditions to the select, you pass it an array, where the array key is the column the condition is based on, and the array value is the value of that column - so a simple select would look something like this:

$theAttributes = array('name','age','height') $theTable = 'people'; $theConditions = array('hair_colour' => 'brown'); $select = $this->executeSelect($theAttributes,$theTable,$theConditions); 

and then $insert[0] would be the number of rows that were selected by the query, and insert[1] would be an array containing each row that was returned as an array.

I'm just wondering if I could get some feedback on it as it seems to work fine for me, but I'm very new to PHP, so some advice such as inefficiency, insecurity, or if it's just plain wrong.

require './config/config.inc.php'; require './config/n_spaces.inc.php'; require_once(__DIR__.'/SqlStatement.php'); require_once(__DIR__.'/MySqlStatement.php'); class PDOdriver { public $conn; public function __construct() { $db_host = DB_HOST; $dbname = DB_NAME; $this->conn = new PDO("mysql:host=$db_host;dbname=$dbname",DB_USER,DB_PASS) or die('Cannot connect to the server, please inform your Network Administrator'); $this->st = new MySqlStatement(); } public function executeInsert($theAttributes,$theTable,$theConditions = NULL) { foreach($theAttributes as $index => $value) { $bind_params[] = ":$index"; } if(!isset($theConditions)){ $sqlstmt = trim($this->st->setTables($theTable)->setAttributes(array_keys($theAttributes))->setValues($bind_params)->makeInsert()); } else { $sqlstmt = trim($this->st->setTables($theTable)->setAttributes(array_keys($theAttributes))->setValues($bind_params)->setConditions($theConditions)->makeInsert()); } if($stmt = $this->conn->prepare($sqlstmt)) { foreach($theAttributes as $index => &$value) { $stmt->bindParam(":$index",$value); } if($stmt->execute()) { $rowCount = $stmt->rowCount(); $success[0] = $rowCount; $success[1] = $this->conn->lastInsertId(); return $success; } else { $success[0] = 'Cannot Execute'; $stmt = NULL; return $success; } } else { $success[0] = 'Cannot Prepare'; $stmt = NULL; return $success; } } public function executeSelect($theAttributes,$theTable,$theConditions = NULL,$fetch_style = "FETCH_ASSOC") { if(is_array($theConditions)) { foreach($theConditions as $index => $value){ $bind_conditions[] = "$index = :$index"; } $sqlstmt = trim($this->st->setAttributes($theAttributes)->setTables($theTable)->setConditions($bind_conditions)->makeSelect()); } elseif(!is_null($theConditions) && !is_array($theConditions)){ $sqlstmt = trim($this->st->setAttributes($theAttributes)->setTables($theTable)->setConditions($theConditions)->makeSelect()); } else { $sqlstmt = trim($this->st->setAttributes($theAttributes)->setTables($theTable)->makeSelect()); } if($stmt = $this->conn->prepare($sqlstmt)) { if(is_array($theConditions)) { foreach($theConditions as $index => &$value) { $stmt->bindParam(":$index",$value); } } if($stmt->execute()) { $success[0] = $stmt->rowCount(); if($success[0] > 0) { if($fetch_style === "FETCH_ASSOC") { while($result = $stmt->fetch(PDO::FETCH_ASSOC)) { $results[] = $result; } $success[1] = $results; } elseif($fetch_style === "FETCH_BOTH"){ while($result = $stmt->fetch(PDO::FETCH_BOTH)) { $results[] = $result; } $success[1] = $results; } elseif($fetch_style === "COUNT"){ } } $stmt = NULL; return $success; } else { $success[0] = 'Cannot Execute'; $stmt = NULL; return $success; } } else { $success[0] = 'Cannot Prepare'; $stmt = NULL; return $success; } } public function executeUpdate($theAttributesColumn,$theAttributesValue,$theTable,$theConditions) { if(is_array($theConditions)) { foreach($theConditions as $index => $value){ $bind_conditions[] = "$index = :$index"; } $sqlstmt = trim($this->st->setUpdateAttributes($theAttributesColumn, $theAttributesValue)->setTables($theTable)->setConditions($bind_conditions)->makeUpdate()); } else { $sqlstmt = trim($this->st->setUpdateAttributes($theAttributesColumn, $theAttributesValue)->setTables($theTable)->makeUpdate()); } if($stmt = $this->conn->prepare($sqlstmt)) { if(is_array($theConditions)) { foreach($theConditions as $index => &$value) { $stmt->bindParam(":$index",$value); } } if($stmt->execute()) { $success[0] = $stmt->rowCount(); return $success; } else { $success[0] = 'Cannot Execute'; $stmt = NULL; return $success; } } else { $success[0] = 'Cannot Prepare'; $stmt = NULL; return $success; } } } 

Answer 1

Long wall of text incoming. Hope it helps :)

First a comment on your file extensions. PHP doesn't care what a file's extension is, it could be an HTML file, text file, or anything. It's just convention that makes us use ".php". Eventually it was determined that adding the ".inc" extension would help distinguish that a file was to be included somehow. We don't have to use this convention, but usually when we do we replace ".php" with ".inc". Seeing them together is just a little odd.

Unless it can't be helped, you should avoid using the require/include_once() functions. These are a bit slower than a regular require/include(). It won't make much difference in such a small application, but if you start going crazy with includes later and are using the _once() versions heavily, you will notice a definite drop in performance.

In PHP there are special kinds of "functions" called language constructs. These functions are native C Language functions that PHP offers directly, without needing to do anything special with them. Theses constructs are generally indistinguishable from traditional functions. The only difference between them is that they are faster and some allow custom syntax that is different from normal function behavior. Your require() functions are so called constructs. So are include()s, echo()s and quite a few others. The point here is that these constructs, as I previously mentioned, have custom syntax. They can be used the same way that you would use a traditional function, or they can use this custom syntax. This custom syntax allows you to neglect adding parenthesis around the construct's parameters. This is a commonly accepted practice. The reason I bring this up is that you are switching back and forth between the two methods. Generally speaking, you should choose a "style" and be consistent with it.

require './config/config.inc.php'; require './config/n_spaces.inc.php'; require_once __DIR__.'/SqlStatement.php'; require_once __DIR__.'/MySqlStatement.php'; //OR require('./config/config.inc.php'); require('./config/n_spaces.inc.php'); require_once(__DIR__.'/SqlStatement.php'); require_once(__DIR__.'/MySqlStatement.php'); 

The or die() short-circuiting syntax is usually frowned upon. The "traditional" way to do it is to wrap it in an if statement and handle the die() portion a little differently. You could have it redirect, throw an error, log an error, try another solution, connect to a default, any combination, or anything else really. But usually die() is thought of as being a very "inelegant" way to approach this. The problem is that many new developers saw this and said, "Oh boy! A quick and easy way!" and immediately adopted it. This is fine for development environments, but in a real life applications customers aren't going to want to see a white page with little or no writing on it.

$this->conn = new PDO("mysql:host=$db_host;dbname=$dbname",DB_USER,DB_PASS); if( ! $this->conn ) { $this->logErr('Cannot connect to the server, please inform your Network Administrator'); } 

Adding "the" to a variable seems odd and unnecessary. Those variables would mean the same thing if typed more simply as $attributes, $table, $conditions. Words like "the" are implied and usually a programmer will add them, or their own favorite placeholder, in the proper place when reading it in their head. This is the hallmark of a beginner programmer who is still trying to make sense of the language by making it read more like plain english. Many of us started off doing this :)

If you need to iterate over an array to read just the keys, then you can use array_keys() and loop over that instead. There is no need to define a variable you aren't going to use. The same goes for iterating over an array to read just the values. So you don't always have to define the keys, or indices, when iterating over an array with a foreach loop.

$attrKeys = array_keys( $theAttributes ); foreach( $attrKeys AS $index ) { 

As you progress in learning the PHP language, you will come across some principles or common practices. One of these principles is called the "Don't Repeat Yourself" or DRY Principle. This states that you should try to make your code as reusable as possible without redundancies. So when you have a variable you need to define that is mostly the same, and only changes a little based on an if/else statement, or some other requirement, you can set it up to be less redundant. For instance:

$sqlstmt = $this->st->setTables($theTable)->setAttributes(array_keys($theAttributes))->setValues($bind_params); if(isset($theConditions)){ $sqlstmt = $this->st->setConditions($theConditions); } $sqlstmt = trim( $sqlstmt . $this->st->makeInsert() ); 

Of course, there is another common practice to do with the above code. It's probably a principle, but I don't know it by name if it is. The closest thing I can think of is the "Single Responsibility" Principle. Anyways, the specific instance that I'm eluding to here is that one line of code should do just one thing. This is to help ensure that your code doesn't become illegible. So, that long $sqlstmt definition should be broken up into a few different parts to make it more legible. For instance.

$attrKeys = array_keys($theAttributes);//can reuse from that loop I mentioned above $table = $this->st->setTables($theTable); $attributes = $this->st->setAttributes( $attrKeys ); //careful, $attributes may conflict with $theAttributes if you remove "the" as I suggested $values = $this->st->setValues($bind_params); if(isset($theConditions)){ $values = $this->st->setConditions($theConditions); } $sqlstmt = trim( $values . $this->st->makeInsert() ); 

Never assign a variable in a statement. This could lead to problems latter during troubleshooting. PHP made a mistake in allowing this syntax, and continues to allow it. IDE's don't know any better because they know PHP says its valid. If you accidentally forget to add two equals signs "==" in a conditional statement you will end up assigning a variable a new value which will cause unexpected results. And, as I just pointed out, your IDE won't know any better because its valid. If you get in the habit of never assigning variables from statements you will know that if you ever see it then you have found your issue. It isn't really foolproof, but it also helps other programmers trying to read your code know where a variable is being declared.

$stmt = $this->conn->prepare($sqlstmt); if( $stmt ) { 

Always define your arrays before using them as arrays. If you don't, you run the risk of changing a string when you really meant to change an array. PHP allows array syntax to be used on strings to access specific characters in a string. For instance in the first part of the following example, $success is a string with the value of "abc". When accessed as an array, the "index" is actually referring to the string's character at that position. So [0] is "a" and [1] is "b". And if you change the character at that position, you change the string. So to avoid this we redefine $success as an array before trying to access it as such to ensure that we haven't accidentally used it somewhere else. We also do this because we should anyways. You are probably receiving silent notices here about $success being undeclared before being used. It isn't enough to crash PHP, but they are still errors and should be avoided.

$success = 'abc'; $success[0] = $rowCount;//$success = $rowCount . 'bc'; $success[1] = $this->conn->lastInsertId();//$success = $rowCount . $this->conn->lastInsertId() . 'c'; //Instead $success = 'abc'; //some code that makes you forget you used $success already $success = array(); $success[0] = $rowCount; $success[1] = $this->conn->lastInsertId(); 

Another thing to note about the above code. When appending onto an array, you do not need to explicitly define the keys being used unless you wish to change a value that already exists, you are creating an associative array, or you want to start counting from a different position rather than "0". If the array $success already exists and we set the value at index "0", then we are changing any value that already exists at that index. If we want to be able to access an array value via key, we usually assign it some associative index to define it, this index is usually descriptive and will make it easy to understand what is being accessed, such as an ID. If we want a numerically indexed array, but don't want to start the index at the traditional "0" position, we can pass an integer to the first element key we add to the array and all indices that occur after that key will be in sequential order starting from that integer. So, if we simply want to append data onto an array we use the following syntax and the numerical keys will be assigned for us.

$success[] = $rowCount; $success[] = $this->conn->lastInsertId(); 

The other two functions appear to be very similar, so the above comments can be used for them too. Eluding back to my comment about DRY. If you find that you are repeating tasks, especially in if/else statements right next to each other, you can make that task into a function to allow for reuse. This will reduce your code size drastically and increase legibility quite a bit too. This isn't just limited to similar if/else statements though. If you have two functions that look very similar you can probably create a third to deal with those similarities. A simple example would be creating a function to create that initial $sqlstmt variable seen as you seem to use it in multiple functions.