MySQLi DB library - quality/security review?

Question

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

<?php
/**
 * @package DbHandler
 * @version 1.04
 * @date 2010-09-27
 * @copyright (c) 2010-2012 Netslik (http://netsilik.nl)
 * @license EUPL (European Union Public Licence, v.1.1)
 *
 * Example of usage:
 * 
 *   // Object creation
 *   $dbPointer = new DbHandler('localhost', 'user', 'secret', 'testDatabase');
 *
 *   // Executing queries
 *   $resultSet = $dbPointer->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) {
            error('No transaction started', E_USER_NOTICE, 1);
            return false;
        }
        $this->_inTransaction = false;
        return $this->_connection->query('COMMIT');
    }

    /** 
     * Execute a prepared query
     * @param string $query the query with zero or more parameter marker characters at the appropriate positions. 
     * Parameter markers are defined as a literal % followed by either 
     * i : corresponding variable will be interpreted as an integer
     * d : corresponding variable will be interpreted as a double
     * s : corresponding variable will be interpreted as a string
     * b : corresponding variable will be interpreted as a blob and should be sent in packets (but this has not yet been implemented)
     * @param mixed $params An optional set of variables, matching the parameter markers in $query. if an array is given, each element 
     * of the array will be interpreted as being of the type specified by the placeholder
     * @throws Exception
     * @return DbResult Object holding the result of the executed query
     */
    public function doQuery($query) {
        $query = trim($query);
        $params = array_slice(func_get_args(), 1);
        array_unshift($params, $this->_parse($query, $params));

        if (strlen($params[0]) <> count($params) - 1) {
            throw new Exception((count($params) - 1).' parameters specified, '.strlen($params[0]).' expected');
        }

        $statement = $this->_connection->prepare($query);
        if ( ! $statement || $statement->errno > 0) {
            throw new Exception('Query preparation failed: '.$this->_connection->error);
        }

        $startTime = microtime(true);
        if ( (strlen($params[0]) > 0 && ! call_user_func_array(array($statement, 'bind_param'), $this->_referenceValues($params)) ) || $statement->errno > 0) {
            throw new Exception('Parameter binding failed');
        }
        $statement->execute();
        $queryTime = microtime(true) - $startTime;

        if ($statement->errno > 0) {
            throw new Exception('Query failed: '.$statement->error);
        }

        return new DbResult( $statement, $queryTime );
    }

    /**
     * Fetch connection resource for this db connection
     * @return resource mysqli connection
     */
    public function getConnectionPtr() {
        return $this->_connection;
    }

    /**
     * Parse the query for parameter placeholders and, if appropriate, match them to the number of elemnts in the parameter
     * @param string &$query a reference to the query string
     * @param array &$params a reference to the array with 
     * @throws Exception
     * @return string all tokens found by the parser
     * @note both the original variables $query and $params passed to this method will be modified by reference!
     */
    private function _parse(&$query, &$params) {
        $s = $d = false; // quoted string quote type
        $n = 0; // parameter index
        $tokenString = '';
        $queryLength = strlen($query);
        for ($i = 0; $i < $queryLength; $i++) {
            switch( $query{ $i } ) {
                case '\\':
                    $i++;
                    break;
                case '\'':
                    if (!$d) { $s = !$s; }
                    break;
                case '"':
                    if (!$s) { $d = !$d; }
                    break;
                case '%':
                    if (!$s && !$d && $i+1 < $queryLength) {
                        if ('i' === $query{$i+1} || 'd' === $query{$i+1} || 's' === $query{$i+1} || 'b' === $query{$i+1}) {
                            if ($n >= count($params)) {
                                throw new Exception('The number of parameters is not equal to the number of placeholders');
                            }
                            if (is_array($params[$n])) {
                                $elmentCount = count($params[$n]);
                                array_splice($params, $n, 1, $params[$n]);
                                $tokenString .= str_repeat($query{$i+1}, $elmentCount);                         
                                $query = substr_replace($query, implode(',', array_fill(0, $elmentCount, '?')), $i, 2);
                                $queryLength += $elmentCount * 2 - 3;
                                $n += $elmentCount;
                            } else {
                                $tokenString .= $query{$i+1};
                                $query = substr_replace($query, '?', $i, 2);
                                $queryLength--;
                                $n++;
                            }
                        }
                    }
                    break;
                // case: check for the various comment start chars (not implemented yet)
            }
        }
        return $tokenString;
    }

    /**
     * PHP >= 5.3 expects the parametrs passed to mysqli_stmt::bind_param to be references. However, we will do the binding after query execution
     * So, this functions quickly solves the issues by wrapping the arguments in an associative array.
     * @param $array
     * @return associative array for PHP >= 5.3, unchanged array otherwise
     */
    private function _referenceValues($array){
        if (strnatcmp(phpversion(),'5.3') >= 0) { // References are required for PHP 5.3+ (and noone knows why)
            $references = array();
            foreach($array as $key => $value) {
                $references[$key] = &$array[$key];
            }
            return $references;
        }
        return $array; 
    }

    /**
     * Rollback a transaction
     * @return bool true on success, false on failure
     */
    public function rollback() {
        if ( ! $this->_inTransaction) {
            error('No transaction started', E_USER_NOTICE, 1);
            return false;
        }
        $this->_inTransaction = false;
        return $this->_connection->query('ROLLBACK');
    }

    /** 
     * Select a database to use on current connection
     * @param string $dbName name of the database to select
     * @return true on success, false otherwise
     */
    public function selectDb ( $dbName ) {
        return $this->_connection->select_db( $dbName );
    } 

    /**
     * Start a transaction
     * @return bool true on success, false on failure
     * @note 'advanced' features such as WITH CONSISTENT SNAPSHOT are not supported
     */
    public function startTransaction() {
        if ($this->_inTransaction) {
            error('Implicit commit for previous transaction', E_USER_NOTICE, 1);
        }
        $this->_inTransaction = true;
        return $this->_connection->query('START TRANSACTION');
    }

    /**
     * Close current db connection on object destruction
     */
    public function __destruct() {
        if ($this->_connection !== null) {
            $this->_connection->close();
            $this->_connection = null;
        }
    }
}

Source: DbResult

<?php
/**
 * @package DbHandler
 * @version 1.04
 * @date 2010-09-27
 * @copyright (c) 2010-2012 Netslik (http://netsilik.nl)
 * @license EUPL (European Union Public Licence, v.1.1)
 *
 */

/**
 * Result object, returned by the DbHandler whenever a valid query is executed
 */
class DbResult {
    protected $_statement = null;
    protected $_result = null;  // holds a bound record as fieldname => value pairs; modified by reference  
    protected $_queryTime = 0;

    /**
     * @param MySQLi $statement Statement the statement for this result set
     * @param int $queryTime the time in seconds the statement took to execute
     * @return object instance
     */
    public function __construct ($statement, $queryTime) {
        $this->_statement = $statement;
        $this->_queryTime = $queryTime;

        if ( false !== ($metaData = $statement->result_metadata()) ) {
            $statement->store_result();
            while ($field = $metaData->fetch_field()) {
                $params[] = &$this->_result[$field->name];
            }
            call_user_func_array(array($this->_statement, 'bind_result'), $params);
            $metaData->close();
        }
    }

    /**
     * get the results for an SELECT statement
     * @return array an associative array per record, containing all fields (columns)
     */
    public function fetch() {
        if (is_null($this->_result)) { // No result data available
            return false;
        }

        $records = array();
        while ($this->_statement->fetch()) {
            foreach($this->_result as $name => $value) {
                $column[$name] = $value;
            }
            $records[] = $column;
        }
        $this->_statement->data_seek(0);
        return $records;
    }

    /**
     * get the results for an SELECT statement
     * @param int $column optional name of the column to fetch
     * @return array an indexed array containing all fields for specified column or the first column if none specified
     */
    public function fetchColumn($column = null) {
        $records = $this->fetch();
        $values = array();
        foreach ($records as $record) {
            foreach ($record as $key => $val) {
                if ( ! is_null($column) && $column <> $key) {
                    continue;
                }
                $values[] = $val;
                break;
            }
        }
        return $values; 
    }

    /**
     * get the results for an SELECT statement
     * @param int $field name of the column of the first record to fetch
     * @return scalar the value of the specified field
     */
    public function fetchField($field, $recordNum = 0) {
        $records = $this->fetch();
        return isset($records[$recordNum][$field]) ? $records[$recordNum][$field] : null;   
    }

    /**
     * get the results for an SELECT statement
     * @param int $recordNum return the num-th record from the set to get
     * @return array an associative array containing all fields (columns) for specified record
     */
    public function fetchRecord($recordNum = 0) {
        $records = $this->fetch();
        return isset($records[$recordNum]) ? $records[$recordNum] : array();    
    }

    /**
     * get the total number of records changed, deleted, or inserted by the last executed statement 
     * @return int number of records affected by this statement
     */
    public function getAffectedRecords() {
        return $this->_statement->affected_rows;
    }

    /**
     * get the number of fields (columns) in the result set
     * @return int number of fields (columns) in the result set
     */
    public function getFieldCount() {
        return $this->_statement->field_count;
    }

    /**
     * get the primary for this inserted record
     * @return int the value of the primary for this inserted record, or 0 if nothing was inserted
     */
    public function getInsertedId() {
        return $this->_statement->insert_id;
    }

    /**
     * get the query execution time
     * @return int time in seconds for this query
     */
    public function getQueryTime() {
        return number_format($this->_queryTime, 4);
    }

    /**
     * get the total number of records in this result set
     * @return int number of records in this result set
     */
    public function getRecordCount() {
        return $this->_statement->num_rows;
    }

    /**
     * dump query result to std-out as an html table
     * @return void
     */
    public function dump() {
        echo "<div class=\"debug\">\n";
        if (is_null($this->_result)) {
            echo "<strong>Query OK, ".$this->getAffectedRecords()." rows affected (".$this->getQueryTime()." sec.)</strong>\n";
        } elseif ( 0 == ($recordCount = $this->getRecordCount()) ) {
            echo "<strong>empty set (".$this->getQueryTime()." sec.)</strong>\n";
        } else {
            $records = $this->fetch();
            echo "<table cellspacing=\"0\" cellpadding=\"3\" border=\"1\">\n";
            for ($i = 0; $i < $recordCount; $i++) {

                if ($i == 0) {
                    $fieldnames = array_keys($records[0]);
                    echo "\t<tr>\n";
                    foreach ($fieldnames as $fieldname) {
                        echo "\t\t<th>".$fieldname."</th>\n";
                    }
                    echo "\t</tr>\n";
                }

                echo "\t<tr>\n";
                foreach ($records[$i] as $field) {
                    echo "\t\t<td>";
                    echo (is_null($field)) ? '<em>NULL</em>' : htmlspecialchars($field, ENT_NOQUOTES);
                    echo "</td>\n";
                }
                echo "\t</tr>\n";

            }
            echo "</table>\n";
            echo "<strong>$recordCount row in set (".$this->getQueryTime()." sec.)</strong>\n";
        }
        echo "</div>\n";
    }

    /**
     * cleanup any unused memory
     */
    public function __destruct () {
        $this->_statement->close();
    }
}

Any and all feedback is welcome.

Answer 1

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 comment, which appears to give me my answer, but I still think the above is valid, it shouldn't need a comment to tell me what a variable is.

I'd use a switch instead of if/else statements. One its cleaner, in my opinion, and two its faster, though that's negligible and isn't a reason to do anything. This is one of those preference things, but one many agree with me on. I've only ever heard one, maybe two people say that switches were harder to read, and I think it probably had more to do with inexperience, or from too much experience with drop cases, for which I understand. In case you're curious, drop cases are where one case purposefully does not break and drops to the next case.

switch( $query{ $i } ) {
    case '\\': $i++; break;
    case '\'':
        if( ! $d ) { $s = ! $s; }
    break;
    case '"':
        if( ! $s ) { $d = ! $d; }
    break;
    case '%':
        if( ! $s && ! $d ) {
        }
    break;
}

Another point for (DRY (Don't Repeat Yourself). You are looking up the next character for the $query string quite frequently. I'd consider making a new variable to hold it instead of typing the same equation again and again to retrieve it. This way you'll reduce the overhead, small as it is, increase the legibility, and should this equation ever change, say you need to skip two places instead of one, you'll be able to do so without needing to change it multiple times.

$nextChar = $query{ $i + 1 };

While I'd still use $nextChar for the rest of the program, I wouldn't use that if statement with all those $nextChar comparisons. You can keep the first part that compares the length, but it would be better to create a whitelist array and use in_array() to determine if the next character is one you want. And best to use two if statements rather than just one, since they check for different things.

$whitelist = array(
    'i',
    'd',
    's',
    'b',
);
if( $i + 1 < $queryLength ) {
    $nextChar = $query{ $i + 1 };
    if( in_array( $nextChar, $whitelist ) ) {

And one more point for DRY. When performing an if/else statement that will change the same variable(s) in both instances, it is much easier to perform similar changes only once. So...

$tokenString .= $nextChar;
$filler = '?';

$contents = $params[ $n ];
if( is_array( $contents ) ) {
    $elmentCount = count( $contents ) - 1;

    $filler = str_repeat( $nextChar, $elmentCount );
    $tokenString .= $filler;

    $queryLength += $elmentCount * 2 - 2;
    $n += $elmentCount;
}

$prevChars = substr( $query, 0, $i );
$restOfQuery = substr( $query, $i + 2 );
$query = $prevChars . $filler . $restOfQuery;

$queryLength--;

$n++;

I moved all of the common bits out of the if/else statements. As such, you'll now notice that the else statement is gone. This is because you did the same thing in the if statement that you did in the else statement, only more so. I had to change some things within the if statement to compensate, but now you only need to change the values once if you ever need to change the base value. The $elementCount now needs to be decremented because everything that uses it has already been incremented. I removed the array_splice() as I could not fathom the reason for it. Correct me if I'm wrong, but doesn't this just remove $params[ $n ] then replace it with the same value? Anyways, after that I created a $filler, using the str_repeat() method you were already using. The other used two functions to accomplish the same thing instead of just one. And I could find no reason for there to be different contents, so I simplified it and just removed the other. After creating the filler, I added it to the $tokenString and later to the $query. I then had to adjust the $queryLength because of the adjustment to $elementCount. BTW: what does this equation mean? I understand the need for doubling, but why the subtraction? I then added the length of $elementCount to $n, which will be incremented once more by default.

Last comment, I think that because you are doing two iterations in one loop, it is better to have both $i and $n declared in the arguments, as you had before, that way everyone knows its purpose.

MAJOR UPDATE

Alright, I've done a bit of research to try and better understand MySQLi and subsequently better answer your questions. As I said before I had no experience with MySQLi. I still have no practical experience with it, but now at least I've got a better understanding of it. So here are a few updates. First some improvements I missed due to lack of understanding.

When creating a new instance of the MySQLi object, you can also immediately select the database. From what I gather, there is no reason to separate this and all the reason to incorporate it. This makes it so that when you check for connection errors you are also ensuring that you are able to select the appropriate database, which you were not doing before. This means a few less lines, one less function to call, and extra protection. I think its a good tradeoff. All you have to do is change the default value of $dbName to an empty string and pass it as the final parameter for the MySQLi constructor. The MySQLi already expects the $dbName to be an empty string by default, so passing an empty string to it shouldn't make a difference.

public function __construct ( $dbHost, $dbUser, $dbPass, $dbName = '' ) {
    $this->_connection = new mysqli( $dbHost, $dbUser, $dbPass, $dbName );

I think its also important to note that, until recently (5.2.9), the connect_error property of the MySQLi object was not working properly and the only way to get this information was to use mysqli_connect_error(). Because it does seem like you are dealing with multiple versions, this could definitely pose a problem.

Now to your last two comments and updated advice.

I'm not sure I understand how the switch breaks the code... The switch is essentially one big if statement that checks the current character. Additional if statements inside the switch serve as AND checking. And all AND checking does is ensure that both sides are TRUE. One side before the other of course, but from what I can see, the order does not appear to make a difference here because you aren't checking for the same character twice, but even if you were, a second if statement in the appropriate case would settle any issues there. The value of the current character is not going to change, so seeing what it is first will not make a difference, so long as we still perform the appropriate checks against it before doing anything with it. Its backwards from how you are currently checking it, but it still follows the same flow. If the current character is a single quote it will still only flip the "quoted string" switch only if the "quoted type" switch has already been flipped. Same for the double quote, and same for the percent sign. They all still rely on those two switches. So, what is different that causes this to break? Is there something I missed? This next bit might actually render this point moot, but I'm still interested.

I don't see how your current array_splice() does anything. Let's take a look at it. First the function in question.

array_splice( $input, $offset, $length, $replacement );

The $input specifies the array, and the $offset specifies index of the desired element to be removed. This is identical to $input[ $offset ]. The $length determines how many elements are going to be removed after the offset. And of course, the $replacement is the replacement element that will take residence in the hole we just chopped out of the array. You give the function an array, an offset, and then a range of offsets, then a replacement element. Pretty simple. So lets look at yours.

array_splice( $params, $n, 1, $params[ $n ] );

We are removing $n from the $params array, or $params[ $n ]. The length is only set to 1 so, this will be the only element that is removed. We are also replacing the element we just removed ($params[ $n ]) with $params[ $n ]. Which is equivalent to saying $params[ $n ] = $params[ $n ] BTW, so the function isn't even necessary here. But do you see how this is redundant? One of us is missing something here. I've had the PHP documentation opened along side me the whole time, so I'm fairly confident in this answer. I don't think this is actually doing anything except taking up overhead, and more overhead than is actually necessary for this task. If you are going to replace a single element of an array, there is no need to use array_splice() to do it. Just reassign the key a new value. But again, even that is unnecessary as it is.

If your program is so expensive that it bogs down with even basic PHP functionality, then there is definitely something wrong. You're either doing too much, or doing something extremely inefficiently. Either way, I would suggest reviewing your code and asking yourself if it can be done better. Of course, that's why you're here. When I originally answered this question I mentioned another way to parse your query, but you shot it down saying what you were doing was necessary. I didn't pursue it further at the time because I didn't understand what you were doing well enough to argue the point. After much examination, and a better understanding, I still think that other way is possible, and better. I've included a snippet of it below. Are you sure you can't do something similar to this? It seems identical to what you are currently doing, but vastly simpler and quicker and would reduce a lot of your overhead. Of course this isn't complete and might require some fine tuning, but it gets the idea across.

while( $pos = strpos( $query, '%' ) ) {
    if( $pos + 1 < $queryLength ) {
        $nextChar = $query{ $pos + 1 };
        if( in_array( $nextChar, $whitelist ) ) {
            $tokenString .= $nextChar;
            $query = substr_replace($query, '?', $i, 2);
        }
    }
}

Also, can't believe I missed this before, but, instead of using two substr() functions to replace a part of a string, why not just use substr_replace() like I have shown above?

referenceValues()

You've broken from your standard practice for private/protected properties/methods. All your other private/protected elements are prepended with an underscore "_". Not everyone does this, each person has their own methods. However I noticed that you were following it up to this point. Why did you stop? Speaking of private/protected. Why are you using protected? Not saying you can't, just that its an odd choice unless you are extending classes.

Getter Methods

I'd personally set up my magic getter method rather than call so many different/specific getter methods. If you want to prevent the certain variables from being retrieve you can create a blacklist for them.

public function __get( $name ) { return isset( $this->_statement->$name ) ? $this->_statement->$name : FALSE; }

dump()

I'd personally remove the HTML from the PHP. It looks very cluttered in this code. Take a look at MVC. You're mostly there already, so adding this bit for HTML output won't take too much effort.

Error Suppressing

You should never suppress errors. Your code should ALWAYS be 100% error free. If something doesn't work without that suppressor, find out why and fix it.

Setting Variables in Statements

I would avoid setting variables in if statements, or any similar statement. They can be a debugging nightmare. For example, I'm not going to tell you what variable you defined, or where, only that its in an if statement, good luck finding it :) Of course, you're probably familiar enough with this program so that finding it probably wont be an issue, but others won't be as fortunate.

Answer:

There are actually multiples of these, but the first is in the first if statement of the DbHandler constructor method.