3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview! Unfortunately your question is off-topic as of now, as the code to be reviewed must be present in the question. Please add the relevant code you want reviewed to your question. \$\endgroup\$ – tim Mar 6 '17 at 20:50
  • \$\begingroup\$ @tim, thank you! i hope question is now in line with the guidelines \$\endgroup\$ – Mohit Srivastava Mar 7 '17 at 9:50
  • 1
    \$\begingroup\$ Yes, thanks. You may also want to adjust the title (and maybe some of the body) of your post, which doesn't really match the question anymore \$\endgroup\$ – tim Mar 7 '17 at 10:03
  • \$\begingroup\$ Thank you again, i hope question is now within proper guidelines \$\endgroup\$ – Mohit Srivastava Mar 7 '17 at 10:10
  • 1
    \$\begingroup\$ Looks good, I voted to reopen \$\endgroup\$ – tim Mar 7 '17 at 10:14
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Thank you for review, i was thinking creating a function function get_error() { return $this->error_reason } and adding error reason in a private variable for easier debugging? returning early does makes things a lot cleaner! \$\endgroup\$ – Mohit Srivastava Mar 7 '17 at 17:52
  • \$\begingroup\$ @MohitSrivastava I personally don't like that approach as checking is still optional (and may thus not be done in many cases), but it is definitely better than nothing. You should also consider using an error class with fixed fields or something along that line (your calling code may want to know what went wrong to decide what to do, and that is a lot easier to do if you do not need to compare to error strings which may be subject to changes). \$\endgroup\$ – tim Mar 7 '17 at 19:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.