mysqli wrapper class

Question

I made the following class to wrap mysqli for PHP using prepared statements. It seems to work well, but I was hoping to get opinions (on overall structure, performance, usage, etc.). Thanks for the insight.

<?php
class database {
    private $conn, $stmt, $arr, $eof=true, $error;

    public function __construct($host, $user, $pass, $db) {
        $this->conn = mysqli_connect($host, $user, $pass, $db);
        if(mysqli_connect_errno()) $this->error = mysqli_connect_error();
    }

    public function __destruct() {
        if($this->stmt) mysqli_stmt_close($this->stmt);
        mysqli_close($this->conn);
    }

    /** Executes a prepared SQL query on the database
      *
      * The second argument (optional) is the types list, followed by the parameters
      * e.g. query('SELECT * FROM table WHERE id = ? OR name = ?', 'is', 5, 'Joe')
      *
      * @param string $query is the sql statement to execute
      * @return mixed false on error,
      *               true on successful select, or
      *               the number of affected rows
      */
    public function query($query) {
        // create a new prepared statement
        $stmt = mysqli_prepare($this->conn, $query);
        if(!$stmt) {
            $this->error = mysqli_error($this->conn);
            return false;
        }

        // apply the arguments if any exist
        if(func_num_args() > 2) {
            $args = array_slice(func_get_args(), 1);
            $refs = array();
            foreach($args as $key=>&$value) $refs[$key] = &$value;
            call_user_func_array(array($stmt, 'bind_param'), $refs);
        }

        // run the query
        $stmt->execute();
        if($stmt->errno) {
            $this->error = mysqli_error($this->conn);
            return false;
        }

        // if the query was not a select, return the number of rows affected
        if($stmt->affected_rows > -1) {
            $rows = $stmt->affected_rows;
            mysqli_stmt_close($stmt);
            return $rows;
        }

        // close any previous statement
        if($this->stmt) mysqli_stmt_close($this->stmt);
        $this->stmt = $stmt;
        $this->eof = false;

        // bind the results to the associative array
        $this->arr = array();
        $refs = array();
        $meta = mysqli_stmt_result_metadata($stmt);
        while($column = mysqli_fetch_field($meta)) {
            $refs[] = &$this->arr[str_replace(' ', '_', $column->name)];
        }
        call_user_func_array(array($stmt, 'bind_result'), $refs);

        // make the first result set available
        $this->next();
        return true;
    }

    // fetches the next row
    public function next() {
        if($this->stmt) {
            $ret = mysqli_stmt_fetch($this->stmt); // populates $this->arr
            $this->eof = ($ret !== true);
            if($ret === false) $this->error = mysqli_error($this->conn);
        }
    }

    // returns an associative array of the results
    public function results() {
        // must make a copy when returning the entire array because
        // the array holds references that may be updated
        $ret = array();
        foreach($this->arr as $key=>$value) $ret[$key] = $value;
        return $ret;
    }

    // return the value for the specified field
    public function result($field) { return $this->arr[$field]; }
    public function __get($field) { return $this->result($field); }

    // returns true if eof has occured or an error
    public function eof() { return $this->eof; }

    // returns the number of rows in the result set
    public function count() { return ($this->stmt ? $this->stmt->num_rows : 0); }

    // returns the last error message, if clear is true, clears the error as well
    public function error($clear=false) {
        $err = $this->error;
        if($clear) $this->error = '';
        return $err;
    }
}
?>

Answer 1

Instead of assigning your properties null or empty values from the start, just define them with no values. If they aren't initialized, then they will resolve to null. Although, from what I can tell, you aren't doing much checking of these properties anyways, so I don't understand why you had default values anyways. You really should be verifying these properties before you use them.

private
    $conn,
    $stmt,
    $arr,
    $eof=true,
    $error
;

You should always specify the access level of your methods and properties (public, private, protected). You did fairly well with your properties, but then you ignore your methods. Sure, right now PHP defaults to making each of these methods public by default, but this will be deprecated before too much longer. Plan for the future and define them properly. Besides, this is just good coding practice.

Always, always, always use braces on your statement. In languages such as Python, where braces don't exist, this is fine, but in PHP this is wrong. Even though PHP allows this syntax, even they say it is wrong, because it CAN cause issues with the code. Not to mention problems with reading the code. Its two characters, and doesn't hurt anything to add them, but it could cause all kinds of problems by not adding them. You do this quite a bit, so I wont point out each case.

if(mysqli_connect_errno()) { $this->error = mysqli_connect_error(); }

When you have a lot to say in the comments, don't use single line comments, use block comments. Or better yet, since you seem to be documenting the behavior of a method here, you should be using PHPDoc comments.

/* returns false on error, true on successful select, number of affected rows otherwise
at minimum, query is the sql statement to execute
if there are query parameters, the second argument is is the types list, followed by the parameters
e.g. query('SELECT * FROM table WHERE id = ?', 'i', 5)*/
//OR PHPDoc Comments
/** Queries database
  *
  * The second argument is is the types list, followed by the parameters
  *
  * e.g. query('SELECT * FROM table WHERE id = ?', 'i', 5)
  *
  * @param string $query At minimum, query is the sql statement to execute if there are query parameters
  *
  * @return mixed false on error, true on successful select, number of affected rows otherwise
  */

You lied to us in the documentation and promised us a third and, at the very least, fourth parameter in your query() method. But they are no where in sight. I do see that you have decided to use func_get_args() in there, but that's a big no-no (at least for something so simple). If you want to pass an unspecified number of parameters, do so as an array. That's all you are really getting with func_get_args() anyways. Either way, your documentation should properly reflect your code, otherwise it is useless. Besides the parameter issue, there is something fundamentally wrong with your query() method. It is entirely too busy. Two very common principles that are always associated with OOP are the "Single Responsibility" (SR) Principle and the "Don't Repeat Yourself" (DRY) Principle, the first isn't normally shortened to that acronym, but I will do so in this answer to avoid repeating myself. These two principles go hand in hand, and you have potentially violated both of them.

SR tells me that this method should only be focused with querying the database and returning the results, after all, that's what you called it query(). However, you are doing so much more. Such as binding the parameters and executing the statement. I wont explain each deviation to you, because you need to be able to separate your code properly, but those two should help get you started. I'm also not entirely certain, but is it necessary for that array to be assigned values by reference? I didn't feel like following it all the way through to make sure.

You have potentially violated the DRY principle because each of the tasks I listed above, and each of the others I didn't, could potentially be reused in this class. Instead they are hardcoded into the one method and therefore their potential is being wasted.

Here's another DRY violator. If you have two methods performing the same task, either A) get rid of one of them, or B) have one of them point to the other. This way if you ever decided to change which array you were fetching from, you would only have to do so once. This is one of the key benefits of DRY.

function result($field) { return $this->arr[$field]; }
function __get($field) { return $this->result($field); }

Ternary functions, unless nested (which they should never be), don't require parenthesis. So, this could be a stylistic point, but figured I'd let you know.

function count() { return $this->stmt ? $this->stmt->num_rows : 0; }