MySQLi DB library - quality/security review?

This libray was written quite some time ago, and it has so far been used in all sorts of small-ish projects.

I'm about to base a more complex, security (Access Control) related, open-source project on this library, so I thought it would be a good moment to ask for a code review.

Source: DbHandler

```
doQuery('SELECT * FROM test WHERE id = %i, 1);
* $records = $resultSet->fetch();
*
* var_dump( $records );
*/
require('class.dbResult.php');

/**
* Database Access abstraction object
*/
class DbHandler {
protected $_connection = null;
protected $_inTransaction;

/**
* @param string $dbHost Either a host name or the IP address for the MySQL database server
* @param string $dbUser The MySQL user name
* @param string $dbPass The MySQL password for this user
* @param string $dbName The optional name of the database to select
* @throws Exception
* @return object instance
*/
public function __construct ($dbHost, $dbUser, $dbPass, $dbName = false) {
$this->_connection = new mysqli($dbHost, $dbUser, $dbPass);
$this->_inTransaction = false;

/* This is the correct OO way to do setup a connection, BUT $connect_error is broken up till PHP 5.3.1.
if ( null !== ($errorMsg = $this->_connection->connect_error) ) {
$this->_connection = null;
throw new Exception('Could not connect to DB-server: '.$errorMsg);
}
*/

// So, let's use a backward compatible check
if (mysqli_connect_error()) {
$this->connection = null;
throw new Exception('Could not connect to DB-server: '.mysqli_connect_error());
}

// Select specified database (if any)
if ( $dbName !== false) {
$this->selectDb( $dbName );
}
}

/**
* Commit a transaction
* @return bool true on success, false on failure
*/
public function commit() {
if ( ! $this->_inTransaction) {

Alright, here we go. There's quite a bit so grab a drink, go to the bathroom, etc...

Unsetting Variables

I don't think unsetting the $dbPass was necessary as it only exists in the local scope of the constructor method, so a var_dump() shouldn't reveal it unless you have a var_dump() in the actual constructor. But I've been known to be wrong. Would be curious to see what others think.

Comments

Your comments are pretty good, maybe not too descriptive, but better than none. So internal comments should not be necessary and will only clutter your code. Trust in your PHPDoc comments and if you need to explicitly explain something, do so there.

Validating Variables

When calling methods to set certain properties or perform certain tasks based on the value of a specific variable, it is better, in my opinion, to perform any validation inside that method rather than before calling it. This has one major advantage. No repetition. Sure, you're probably not going to change your $dbName again, but if you wanted to, or if you found yourself reusing this code, such as extending it, then you'd have to write that check over again. Better to have the method perform its own validation.

Public vs Private

I'm not sure that your selectDb() method should be public. I would think that the database should not change in the middle of an operation, so it should be private to prevent accidental, or malicious, changes. If you need to access another database, then you should create a new instance, not change the existing one.

Diamond Operators * UPDATE

I don't think this is supported... Never seen it in any PHP code before, though I do remember seeing it somewhere before, which is the only reason I knew to call it a diamond operator.

if (strlen($params[0]) <> count($params) - 1) {

EDIT: You've proven it is supported, and I swear I looked at that page, but anyways, still looks odd, but to each their own.

Long Statements

Its best to either separate long statements, or abstract their arguments. In other words, if the statement is too long break it up into multiple statements or for those arguments that aren't already variables, make them so.

$length = strlen( $params[ 0 ] );
$bindParams = call_user_func_array(
    array( $statement, 'bind_param' ),
    $this->referenceValues( $params )
);
if ( $length > 0 && ! $bindParams || $statement->errno > 0) {

Repetitive Tasks

Best to create functions for those repetitive tasks. Serves two purposes. First, less typing. Second, should you ever want to change how that task is performed, best to only have to change it once. So everywhere you call the error() function, which seems odd after all those exceptions, could be more easily written with a function similar to so.

function throwError( $method ) { error( "$method: No transaction started", E_USER_NOTICE, 1); }
//example usage
$this->throwError( __METHOD__ );

_parse() * UPDATE

This function is way too busy. There's too much going on. Why is the query being trimmed? That has nothing to do with parsing and should have been done prior to calling this function. Doing so will also remove the need to assign that argument by reference. Speaking of which reference assignments make troubleshooting a nightmare. If you don't know this code, you aren't going to realize, without tracking it down, that the variable changed. What is $s? What is $d? Why did you assign them that way? Sure it means you can do them both on the same line, but for legibility they should definitely both be defined separately. Why are there two iterators in one array? If you separate these into different loops, you'll be halfway to separating this into two different functions.

I'm not really sure how I feel about the string position array syntax. But why are you using braces "{}" instead of brackets "[]"? I didn't even know that was possible. Also, is it really necessary to go over every character in that string? From the looks of it you only want to perform certain actions based on the first couple of characters and the rest are empty spaces. I'd locate those few characters, set them as variables, then proceed to loop over the $params. This seems like entirely too much effort was put into a simple task.

EDIT: I still think some of the above is valid, which is why I'm leaving it, that and to give a reference to the earlier discussions. Here's the update, which is about as long as the entire first update.

Still not sure what $s and $d are. They are too vague. I can tell they are switches that are toggled based on the others value, but what are they switches for? If you can explain their purpose, you'll have a better idea what to call them. Self documenting code is easily read and easily understood and should be something everyone strives for in my opinion. You've mostly done this, this is just one of the few times you didn't. And as a consequence, I have no idea what's supposed to be going on here. I just noticed your commen

StackExchange Code Review Q#13352, answer score: 10