Implementation of a Custom Database Class

Question

Following Database Class is what I'm using as a wrapper to connect with MySqli in a application, and below the class is how i'm implementing it currently I'm not sure if this is the right way to handle it or how I can further improve.

class Database {
/**
 * @var $conn: type Mysqli
 **/

private $conn = null;
private $stmt = null;

/**
 * @__construct: establishes a new Connection to db
 **/

function __construct() {
    $this->conn = new mysqli(
        DatabaseHelper::DB_HOST,
        DatabaseHelper::DB_USERNAME,
        DatabaseHelper::DB_PASSWORD,
        DatabaseHelper::DB_NAME 
    );
}

/**
 * @ret: $conn
 **/
public function connect() {
    return $this->conn ? true : false;
}

public function prepare($query) {
    $this->stmt = $this->conn->prepare($query);
    if ($this->stmt) {
        return true;
    } else {
        return false;
    }
}

public function set_params($sql_str, ...$params) {
    // bind the sql string
    $bind_args = array();
    $bind_args[] = $sql_str;

    foreach ($params as $param_key => $param_val) {
        $bind_args[] = &$params[$param_key]; // pass the reference to bind_Args
    }

    if (call_user_func_array(array($this->stmt, 'bind_param'), $bind_args)) {
        return true;
    } else {
        return false;
    }
}

public function exec_query() {
    $ret = $this->stmt->execute();
    $this->stmt->close();
    return $ret ? $ret : false;
}

public function exec_query_array() {

    $ret = $this->stmt->execute();
    if ($ret) {
        $this->stmt->store_result();
        $response = array();
        while($r = $this->getResultByArray($this->stmt)) {
            $response[] = $r;
        }
        $this->stmt->close();

        return $response;
    } else {
        return false;
    }
}

private function getResultByArray($stmt) {
    if($stmt->num_rows>0) {
        $result = array();
        $md = $stmt->result_metadata();
        $params = array();
        while($field = $md->fetch_field()) {
            $params[] = &$result[$field->name];
        }
        call_user_func_array(array($stmt, 'bind_result'), $params);
        if($stmt->fetch())
            return $result;
    }
    return null;
}}

and accessing it in this way

 public function findUserByMobile($mobile) {
    $db = new Database();
    $query = "SELECT * FROM users WHERE mobile = ?";
    $sql_str = "s";
    if ($db->connect()) {
        if ($db->prepare($query)) {
            if ($db->set_params($sql_str, $mobile)){
                $ret = $db->exec_query_array();
                if ($ret) {
                    return $ret;
                } else {
                    return false;
                }
            } else {
                return false;
            }
        } else {
            return false;
        }
    } else {
        return false;
    }
}

This code is also available here.

Answer 1

As I've got some experience with mysqli, I can review not only the code structure but also the actual functionality as well.
There is some room for improvement, but first of all I must mark three rather critical issues (that are highlighted in my article, Your first database wrapper's childhood diseases, which, although based on PDO, nevertheless is I recommend to read).

1. A new connection to database is created every time a function is called. It will kill your database server. A connection should be made only once.
2. There is no error reporting that will leave you unaware of all possible errors, either during development or on the production server.
3. Your class is stateful. It means it keeps some state between calls, but it should not. There should be no $stmt class variable or you'll get into a big trouble. Imagine you are processing some query's result in a loop and going to run another query inside. A nested query call will override all your stateful variables, wrecking havoc. Just look at PDO and mysqli: both use a separate object to hold all the stuff connected to a particular query.

These issues should be fixed obligatory.

Now to improvements.

• connect() method is rather useless. There is no situation where it is supposed to return false.

• you are already using argument unpacking operator in your code - so you don't have to meddle with call_user_func(), making your set_params() method as simple as

  public function set_params($stmt, $types, ...$params) {
      $stmt->bind_param($types, ...$params);
  }
  

• so now you can tell that three methods of your class are plain duplicates for the original mysqli methods. Which, in turn, raises a question - whether you need to duplicate them? Well, in theory, it's better to make your class to duplicate 100% of mysqli functionality, making the abstraction solid. But in reality it would be too hard a task. So I would suggest to stick with a leaky abstraction, falling back to mysqli's methods when needed. While to your class I'd suggest to add only methods that do not exist in mysqli.
• and here is a case for such a method: in the findUserByMobile() method you are calling prepare, bind and execute in order. I bet you are doing it for the every query you run. Why not to encapsulate all three into a helper method?
• there is no real need to supply a parameter type every time you bind a variable. A string type almost always will do. So let's borrow such a helpful feature from PDO, which lets you to bind variable without setting the type explicitly, making them all string by default.
• exec_query_array() / getResultByArray() are fair, but only if your PHP doesn't support mysqli_get_result() / mysqli_fetch_all() methods, which it should. So let's use the existing PHP functionality and won't reinvent the wheel.
• BTW, be consistent with method naming. I suppose you took getResultByArray somewhere else - it's ok for the content but you are supposed to rename it into something like get_array_result()

So, what we can make from your two classes?

class Database {

    public $conn = null;

    function __construct()
    {
        mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
        $this->conn = new mysqli(
            DatabaseHelper::DB_HOST,
            DatabaseHelper::DB_USERNAME,
            DatabaseHelper::DB_PASSWORD,
            DatabaseHelper::DB_NAME             );
    }

    public function exec_query($sql, $params = [], $types = NULL)
    {
        if (!$params)
        {
            return $this->conn->query($sql);
        }
        if (!$types)
        {
            $types = str_repeat("s", count($params));
        }
        $stmt = $this->conn->prepare($sql);
        $stmt->bind_param($types, ...$params);
        $stmt->execute();
        return $stmt->get_result();
    }
}

here we left only a constructor and a helper method to run prepare/bind/execute in a single call. Also $conn variable is made public, in case we will need some mysqli related function. Note the mysqli_report() function that will tell mysqli to report its errors which is critically important.

class User {

    protected $db;

    public function __construct($db)
    {
        $this->db = $db;
    }

    public function findUserByMobile($mobile)
    {
        $query = "SELECT * FROM users WHERE mobile = ?";
        return $this->db->exec_query($query, [$mobile])->fetch_assoc();
    }
}

Here, you see, we made a database connection as a class variable set in constructor. While findUserByMobile() became as simple as just two lines, thanks to our helper function.

So here is how it works:

$db = new Database();
$user = new User($db);
$userData = $user->findUserById($mobile);

As you can see, your code could be made a lot shorter and much more flexible.

The last part is error reporting. It is not directly related to a Database class, as such a class should only raise an error, but shouldn't report it itself. While error reporting should be delegated to a site-wide error handler. There are two main principles for such a handler:

• A programmer should be notified of the every single error that occurs
• Not a single system error should be leaked to a user. Only a generalized error message should be sent.

So for this purpose a simple code could be used (again taken from my article above):

set_error_handler("myErrorHandler");
function myErrorHandler($errno, $errstr, $errfile, $errline)
{
    error_log("[$errno] $errstr in $errfile:$errline");
    header('HTTP/1.1 500 Internal Server Error', TRUE, 500);
    header('Content-Type: application/json');
    echo json_encode(["error" => 'Server error']);
    exit;
}

For the every error occurred in your script it will log it in the error log, while a generalized message is sent to the client.

Some statements could be not that clear, due to vast amount of information I had to deliver. But I'll be glad to answer whatever questions you will have.

Answer 2

Returning Early / Guard Clauses

Returning early is very easy to do and can severely improve your code.

Looking at findUserByMobile, it is nearly impossible to say which else closes which if on first glance. In this case it doesn't matter that much as all else do the same thing, but it is still not nice to read.

If you just invert the if and return, the code is a lot nicer:

 public function findUserByMobile($mobile) {
    $db = new Database();
    if (!$db->connect()) {
        return false;
    }

    $query = "SELECT * FROM users WHERE mobile = ?";    
    if (!$db->prepare($query)) {
        return false;
    }

    $sql_str = "s";
    if (!$db->set_params($sql_str, $mobile) ){
        return false;
    }

    $ret = $db->exec_query_array();
    if (!$ret) {
        return false;
    }
    return $ret;
}

If you don't care what went wrong, you could also combine some of the ifs. In practice, you probably do care though.

Error Handling

Just returning false on error can make debugging quite difficult. Did the query fail because you couldn't connect to the database? Because the query is wrong? Because the parameter is wrong? Because no result could be found? We just don't know. Think about throwing exceptions instead.

Database connections

You really do not want to create a new database connection for each query. Instead, pass the database object to the method and reuse it.

Misc

• connect sounds like it would connect something. Instead, it returns something. It is customary to name the method isConnected.
• don't shorten variable names. ret is more difficult to read than return or result, metadata is better than md, etc.
• always use curly brackets, even for one-line statements.