5
\$\begingroup\$

Please let me know if any improvements could be made to my PHP PDO MySQL Class.

/**
*
* Database.
*
* Version 1.2 (March 2017).
*
**/

class Database {

    // Connection information.
    private $connection;

    // SQL querey information.
    private $querey;

    // Connected to the database server.
    private $connected = false;

    // Errors.
    private $error;

    // Hostname or IP address of the database server.
    private $host = "127.0.0.1";

    // Port to access the database server.
    private $port = 3306;

    // Name of the database.
    private $database = "test";

    // Username.
    private $username = "test";

    // Password.
    private $password = "qgmxnMVroz9PHTY36Eq4qwpA5GSXwFyb"; // This is a random password!

    // Database charset.
    private $charset = "UTF8";

    // PDO options.
    private $options = [
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES => false,
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_PERSISTENT => true
    ];

    /**
    *
    * Constructor.
    *
    * Creates connection to the database server.
    *
    **/

    public function __construct() {

        if ($this->connected === true) {

            return true;

        } else {

            try {

                $this->connection = new PDO("mysql:host={$this->host};port={$this->port};dbname={$this->database};charset={$this->charset}", $this->username, $this->password, $this->options);

                $this->connected = true;

            } catch (PDOException $e) {

                $this->error = $e->getMessage();

                return null;

            }

        }

    }

    /**
    *
    * Query the Database.
    *
    * Used for SELECT, INSERT, UPDATE and DELETE statements.
    *
    **/

    public function query($query, $parameters = [], $expectSingleResult = false) {

        if ($this->connected === true) {

            if (is_string($query) && $query !== "" && is_array($parameters) && is_bool($expectSingleResult)) {

                try {

                    // Prepare SQL querey.
                    $this->querey = $this->connection->prepare($query);

                    // Bind parameters to SQL querey.
                    foreach ($parameters as $placeholder => $value) {

                        // Parameter type.
                        if (is_string($value)) {

                            // Parameter is a string.
                            $type = PDO::PARAM_STR;

                        } elseif (is_int($value)) {

                            // Parameter is a integer.
                            $type = PDO::PARAM_INT;

                        } elseif (is_bool($value)) {

                            // Parameter is a boolean.
                            $type = PDO::PARAM_BOOL;

                        } else {

                            // Parameter is NULL.
                            $type = PDO::PARAM_NULL;

                        }

                        // Bind parameter.
                        $this->querey->bindValue($placeholder, $value, $type);

                    }

                    // Execute SQL querey.
                    $this->querey->execute();

                    // Get Result of SQL querey.
                    if ($expectSingleResult === true) {

                        $results = $this->querey->fetch();

                    } else {

                        $results = $this->querey->fetchAll();

                    }

                    // Return results of SQL querey.
                    return $results;

                } catch (PDOException $e) {

                    $this->error = $e->getMessage();

                }

            } else {

                $this->error = "Invalid Querey or Paramaters";

                return null;

            }

        } else {

            $this->error = "Not Connected to Database Server";

            return null;

        }

    }

    /**
    *
    * Row count for the last querey.
    *
    **/

    public function rowCount() {

        if ($this->connected === true) {

            return $this->querey->rowCount();

        } else {

            $this->error = "Not Connected to Database Server";

            return null;

        }

    }

    /**
    *
    * Get ID for the last querey.
    *
    **/

    public function lastId() {

        if ($this->connected === true) {

            return $this->connection->lastInsertId();

        } else {

            $this->error = "Not Connected to Database Server";

            return null;

        }

    }

    /**
    *
    * Begin a transaction.
    *
    **/

    public function beginTransaction() {

        if ($this->connected === true) {

            return $this->connection->beginTransaction();

        } else {

            $this->error = "Not Connected to Database Server";

            return null;

        }

    }

    /**
    *
    * Rollback and cancel/end a transaction.
    *
    **/

    public function cancelTransaction() {

        if ($this->connected === true) {

            return $this->connection->rollBack();

        } else {

            $this->error = "Not Connected to Database Server";

            return null;

        }

    }

    /**
    *
    * Or...
    *
    **/

    public function rollbackTransaction() {

        if ($this->connected === true) {

            return $this->connection->rollBack();

        } else {

            $this->error = "Not Connected to Database Server";

            return null;

        }

    }

    /**
    *
    * Commit and end a transaction.
    *
    **/

    public function endTransaction() {

        if ($this->connected === true) {

            return $this->connection->commit();

        } else {

            $this->error = "Not Connected to Database Server";

            return null;

        }

    }

    /**
    *
    * Close the current connection the the database server.
    *
    **/

    public function close() {

        $this->connection = null;

    }

}

?>

In particular should I remove the if ($this->connected === true) blocks?

\$\endgroup\$
  • \$\begingroup\$ To provide usage examples would be a very good idea. \$\endgroup\$ – Your Common Sense Mar 5 '17 at 14:13
  • 1
    \$\begingroup\$ I just updated my answer, with improved examples and one addition to the class. \$\endgroup\$ – Your Common Sense Mar 6 '17 at 8:42
3
\$\begingroup\$

Curiously, I already wrote a review for your class quite a while ago, Your first database wrapper's childhood diseases - so common the mistakes everyone make with their first PDO wrapper.

In short, your wrapper adds very little to the original PDO, at the same time severely limiting PDO's features. I would strongly recommend to get rid of this class, and stick to the original PDO with only one addition, as your query() method is indeed a good idea. So I would make it just

class MyPDO extends PDO
{
    public function run($sql, $args = NULL)
    {
        if (!$args)
        {
            return $this->query($sql);
        }
        $stmt = $this->prepare($sql);
        $stmt->execute($args);
        return $stmt;
    }
}

this general purpose query function is called "run" not to be confused with the original PDO::query().

You'd be surprised, but this micro-wrapper does everything that your class do, and also a lot more!

For example, original PDO let you prepare your query once, and then execute it multiple times. Not that this feature is extremely on demand, but still.

At the same time, PDO allows you to get the resulting data in dozens of different formats, simply by returning the PDOStatement.

Update.

On a second thought I think that to keep such an array with default settings would be a good idea. So it's better to make your class like

class MyPDO extends PDO
{

    public function __construct($dsn, $username = NULL, $password = NULL, $options = [])
    {
        $default_options = [
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_EMULATE_PREPARES => false,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        ];
        $options = array_merge($default_options, $options);
        parent::__construct($dsn, $username, $password, $options);
    }
    public function run($sql, $args = NULL)
    {
        if (!$args)
        {
            return $this->query($sql);
        }
        $stmt = $this->prepare($sql);
        $stmt->execute($args);
        return $stmt;
    }
}

But for the rest - I honestly cannot think of any improvement else.

Yes, you'll have to supply the connection credentials manually, but that's actually a good thing - it will make your class reusable. Just include it into another project and instantiate with different credentials! Or you can use another connection in the same project, if required.

So I would strongly suggest such a mini-extension only. Now to the code review.

  1. Constructor. Indeed, I don't really get why you're running $this->connected === true blocks everywhere. Any condition when the result could be anything else? Get rid of them.
  2. Catching exceptions on-site. Exceptions is a great mechanism, allowing clean and sensible error handling and reporting. Your code diminishes them to blunt errors. What if some your application code would expect a certain exception? How it would be possible to catch it? Leave exceptions alone, don't catch them.
  3. Function query(). This one is the biggest flaw, consists of many:
    • In fact, in some circumstances an attempt to use it with a query other than SELECT would produce an error, as you cannot fetch from INSERT or DELETE.
    • It won't work with any query that cannot be prepared, CREATE TABLE for example
    • it does a lot of useless stuff with binding, while all you need to do is just send your $parameters into execute.
    • Really, the number of possible result formats is not limited to just single row and array of rows. What about single cell result? A 1-dimensional array? Key-value pairs? An indexed list? A grouped list? Everything is possible with vanilla PDO but impossible with your rather blunt query() function
    • When working with large datasets you cannot use fetchAll() and should fetch the rows one by one. Your function doesn't allow it.
    • It's impossible to call a stored procedure using this function (or, strictly speaking, impossible to run any other query after calling a stored procedure).
    • this function won't work with positional placeholders

All these drawbacks can be fixed with two simple improvements already used in the micro-"wrapper" above:

  1. Just let your function to run the original PDO::query() if no parameters are supplied
  2. Return PDOstatement, from which any desired result can be accquired using the method chaining.

This done, you can run whatever kinds of queries exist and get whatever results supplied by PDO. Some examples (given $db is a working instance of MyPDO wrapper) are:

$db = new MyPDO("mysql:host=$host;dbname=$db;charset=$charset", $user, $pass);

# Table creation
$db->query("CREATE temporary TABLE pdowrapper (
            id int auto_increment primary key, name varchar(255))");

# Prepared statement multiple execution
$stmt = $db->prepare("INSERT INTO pdowrapper VALUES (NULL, ?)");
foreach (['Sam','Bob','Joe'] as $name)
{
    $stmt->execute([$name]);
}
var_dump($db->lastInsertId());

# Getting rows in a loop
$stmt = $db->run("SELECT * FROM pdowrapper");
while ($row = $stmt->fetch(PDO::FETCH_LAZY))
{
    echo $row['name'],",";
    echo $row->name,",";
    echo $row[1], PHP_EOL;
}

# Getting one row
$id  = 1;
$row = $db->run("SELECT * FROM pdowrapper WHERE id=?", [$id])->fetch();
var_export($row);

# Getting single field value
$name = $db->run("SELECT name FROM pdowrapper WHERE id=?", [$id])->fetchColumn();
$count = $db->run("SELECT count(*) FROM pdowrapper")->fetchColumn();
var_export($name, $count);

# Getting array of rows
$all = $db->run("SELECT name, id FROM pdowrapper")->fetchAll(PDO::FETCH_KEY_PAIR);
var_export($all);

# Update
$new = 'Sue';
$stmt = $db->run("UPDATE pdowrapper SET name=? WHERE id=?", [$new, $id]);
var_dump($stmt->rowCount());

The listing above is the actual working code that can be run as is.

\$\endgroup\$
2
\$\begingroup\$

I think you already have a great answer about the general approach you took.

I'll focus on the readability of your code, as there are some very simple things that you can do to severely increase it.

Paragraphs

Currently, almost every line is on its own paragraph, resulting in way too much vertical whitespace.

Your query function for example is 82 lines. Just by using sensible paragraphs (eg no new line after { or before }) you can get that down to about 50 lines.

If Structure

When writing an if, it is generally a good idea to put the case that contains less lines first. It is really difficult to find the corresponding else when you can't even fit both of them on the screen.

Often it also makes sense to return in the else case, thus avoiding deep nesting.

Comments

Your comments shouldn't simply repeat what your code is already telling us. Comments like that do not add any information and distract from the actually relevant comments.

Examples are Return results of SQL querey., Execute SQL querey., etc.

Exceptions

There is no need to catch exceptions if you can't actually handle them. Just let them bubble up until you actually do know what to do.

And if you do use PDO exceptions, you might as well also use exceptions for your own error messages. It is preferable to returning null and setting an error message (which the caller may or may not check).

If you follow all that, your query method would now look like this, which I think is a lot more readable:

public function query($query, $parameters = [], $expectSingleResult = false) {
    if ($this->connected !== true) {
        throw new NotConnectedException("Not Connected to Database Server");
    }

    if (!is_string($query) || $query === "" || !is_array($parameters) || !is_bool($expectSingleResult)) {
        throw new InvalidArgumentException("Invalid Querey or Paramaters");
    }

    $this->querey = $this->connection->prepare($query);

    foreach ($parameters as $placeholder => $value) {
        if (is_string($value)) {
            $type = PDO::PARAM_STR;
        } elseif (is_int($value)) {
            $type = PDO::PARAM_INT;
        } elseif (is_bool($value)) {
            $type = PDO::PARAM_BOOL;
        } else {
            $type = PDO::PARAM_NULL;
        }
        $this->querey->bindValue($placeholder, $value, $type);
    }

    $this->querey->execute();

    if ($expectSingleResult === true) {
        return $this->querey->fetch();
    } else {
        return $this->querey->fetchAll();
    }
}
\$\endgroup\$

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.