PDO wrapper class for database

Question

I have my PDO Wrapper class and would love to get some more tips/opinions about what you think. This is my first class and I basically put my time into making it as logical and standardized as possible. I know I need to add comments.

<?php
/*
    COPYRIGHT © SIMPLEBB.NET
    CREATED BY: MikeM.
    CREATED IN: 2014
    COPYRIGHT © SIMPLEBB.NET
*/
class Database{
    private $db;

    public function __construct($config){
        $driver = strtolower($config['database']['driver']);
        $host = $config['database']['host'];
        $database = $config['database']['name'];
        $username = $config['database']['username'];
        $password = $config['database']['password'];

        $this->connect($driver, $host, $database, $username, $password);
    }

    private function connect($driver, $host, $database, $username, $password){
        try{
            $this->db = new PDO($driver.":host=".$host.";dbname=".$database, $username, $password);
            $this->db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); 
            $this->db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }catch(PDOException $ex){
            $errorCode = $ex->getCode();
            $errorMessage = $ex->getMessage();
            $errorFile = $ex->getFile();
            $errorLine = $ex->getLine();
            include(dirname(dirname(__FILE__))."/error.php");
        }
    }

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

    public function nonQuery($query){
        try{
            $run = $this->db->prepare($query);
            if(func_get_args() > 0){
                $args = func_get_args();
                array_shift($args);
                return $run->execute($args);
            }else{
                return $run->execute();
            }
        }catch(PDOException $ex){
            $errorCode = $ex->getCode();
            $errorMessage = $ex->getMessage();
            $errorFile = $ex->getFile();
            $errorLine = $ex->getLine();
            include(dirname(dirname(__FILE__))."/error.php");

        }
    }

    function query($query){
        try{
            $run = $this->db->prepare($query);
            if(func_get_args() > 0){
                $args = func_get_args();
                array_shift($args);
                $run->execute($args);
            }else{
                $run->execute();
            }   
            return $run; // using: ->fetchall(PDO::FETCH_ASSOC); needs: print_r($db->query("SELECT * FROM `test2`"));
        }catch(PDOException $ex){
            $errorCode = $ex->getCode();
            $errorMessage = $ex->getMessage();
            $errorFile = $ex->getFile();
            $errorLine = $ex->getLine();
            include(dirname(dirname(__FILE__))."/error.php");
        }
    }
}
$db = new Database($config);
/*
    COPYRIGHT © SIMPLEBB.NET
    CREATED BY: MikeM.
    CREATED IN: 2014
    COPYRIGHT © SIMPLEBB.NET
*/
?>

I'd recommend to do:

$db = new Database($config);

only when you really need the database class, you don't want un-closed connections to be open. You can let this the same but make sure you manually do the connection when you need it!

Therefore this is globalized error logging. In some cases you do not want that so I'd recommend for such cases to destroy these try catches and actually do that when you call the function.

Answer 1

This review will consist of 2 parts:

1. The design/concept
2. The code itself

1: Code should solve problems

Every piece of code you write and will ever write should solve a problem. If it is not a solution to a problem you have. Throw it away. Eventually it will break, and then kittens will die!

To make sure we don't kill to many kittens, we have to write good, solid and simple code. Code that everyone understands, even 'me'-in-5-days-with-a-hangover. Some very smart geeks have even made a couple of rules: SOLID, KISS ...

Let's look at your class. It's name is Database. That tells me that your class represents a Database. In orde to function, it will probably need a DatabaseConnection, and maybe even a LoggerInterface (psr-3). But thats just a guess. It will probably help me in creating transactions, flushing the data, error-retrieval, ...

I am so wrong

The Database object needs 1 parameter. It's called $config, sadly, the documentation is simply missing. So I start analysing the code. Apparantly it needs $driver, $host, $database, $username, $password. So why not ask for it? Really. What is more user friendly:

$dbConnection = new Database(array('config'=>array('username'=>'',...)));

or

$dbConnection = new Database('username','password',...);

If you need something, ask for it.

So what problem are you trying to solve?

One problem could be the creation of DatabaseConnections. This could be solved using a DatabaseConnection Factory. Using the factory pattern that is.

Or maybe, you are trying to create some kind of DBAL? A class that uses a DatabaseConnection and performs queries.

Or maybe you wanted to create an error-logger/catcher (looking at all those try/catch)?

Different problems need differen solutions

Every class should solve 1 problem. At max.

2: Code should be stupid

If code doesnt read like normal language or poetry, it is bad code. If code does read like normal language, read again to check if it makes sence. an example:

class Cabin
{
    public function open() {
        //the cabin is now open
    }
}

Looks perfectly normal.

$myCabin = new Cabin();
$myCabin->open();
//we can now put stuff in the cabin

Read well, everyone knows whats going on. Now read it again. The actor is the acinb, the action is open.You open something, so what is the cabin opening? Somehow, we wrote code where the Cabin is opening itself, magically. Better would be:

$cabinHandler->openCabin($cabin);
if ( $myCabin->isOpen() ) {
    //we can no put stuff in the cabin
}

A more real-world example:

$user->resetPassword();
$user->setResetPasswordMail();

Looks normal, right? But why does the User know how to send emails? Better:

$email = new Email($resetPasswordEmailContent)
$mailer->sendEmail($email, $user);

Now back to your Database class. Let's just say you went for the DBAL solution. We start with the __construct. What is the exact minimum requirement? a connection. We use pdo:

//name is not perfect
class Database
{
    public function __construct(PDO $dbh)
    {
        $this->dbh = $pdo;
    }
}

We also want to abstract away the preparing, and statement handling etc etc

/**
 * Query the database connection and return the result
 * @param  {String} $sql            [description]
 * @param  {Array} $values=array() [description]
 * @return {Array}                 [description]
 */
public function query($sql, $values=array())
{
    try {
        $sth = $this->dbh->prepare($sql);
        $sth->execute($values);
    } catch (PDOException $e) {
        $this->logger->critical($e->getMessage, array($sql, $values));
        throw new DatabaseException('An error occured performing a query');
    }

    return $sth->fetchAll();
}

So instead of failing miserably on an exception. We log our PDO exception (We use PDO so we can know about it). And then throw e new more generic error message. The code that then called us can choose 2 things on an error: 1) ignore or 2) catch it and try and fix the problem.

Eventually, if there is no caller up the chain that actually knows what to do with this Excpetion. The DatabaseException will be caught by your error_handler. This one will simply show a nice error page with some generic error message. For instance the google-monkeys.

Now that we have established how we should code. Let's look at how the SOLID rules could help us.

S : Single responsibility principle

Every piece of code should have one single responsibility. This goes for Objects and for methods. The class DatabaseHandler should only handle the Database - or act as an abstraction. Every method should only do one thing: perform a query, open a transaction, close a transaction. It shouldn't perform a query AND handle the exceptions AND render the error-view (include html stuff).

O : open/closed principle

Every module - read piece of code - should be open for extension but closed for modification. This means that we should be able to change the behaviour without changing the source code itself. Your Database constructs the dsn for the PDO object. But what if you want to use sqlite instead of mySQL? Your Database class does not handle this case. So for changing the behaviour, we need to change your SourceCode.

:(

That is my sad face

L : Liskov substitution principle

Not applicable here since you are not extending classes. But it is a nice principle to look into when you start building very large aplications.

I : Interface segregation principle

Only ask what you need. And not only for variables, but for methods aswell. In your construct you are asking for a $config array. By just looking at it I know there is more then just the $config['database'] in there. Why? The database should care less about other config stuff. It shouldn't get them. It shouldn't even know that you are passing config around in an array. The same applies when using other objects.

Your problem with the SQLite db can forinstance be solved with Typehinting for an interface instead of a class (PDO in your case). You are only using the prepare($query) method. So why ask for an entire PDO object? What if we want to change PDO with something different - you never know, PDO could be the biggest kitten killer out there. So create an interface and ask that:

interface DatabaseConnection
{
    /**
     * @param  {String} $query The query with placeholders
     * @return {StatementHandler}
     */
    public function prepare($query);
}

and then typehint for this DatabaseConnection:

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

Now everyone who is going to use your code knows what a DatabaseConnection should look like. A very easy example is the following one:

class PdoDatabaseConnection extends PDO implements DatabaseConnection {}

And now everyone is happy. Even the guy who is writing the dreadfull MysqlDatabaseConnection with all it's mysql_query glory

D : Dependency inversion principle

Your connect() function is a good example of how NOT to do it. What happens when you construct your database object is the following:

1) get some parameters from an array into a local variable (why? i dont know) 2) pass those variables to the connect function 3) try to connect to the database with the config/credentials provided 4) if you fail, handle the error and show an error screen.

That's a lot. To much. Your abstract 'Database' class that should have abstracted the hard PDO object, know completly depends on that PDO object. It even knows how to create it. While all the class needs it some object where he can call ->prepare() and then get a Statement back that he can ->execute() and ->fetchAll().

Everytime you find yourself writing new inside a class. And the class doesn't end witht he word Factory. Stop what you are doing and ask for an interface instead.

There are ofcourse exceptions. For instance PDO. The DatabaseConnection interface will be overkill in most real world examples.

---

Know we have established the SOLID rules, there still remains DRY: Don't Repeat Yourself. Your Database class has 5 methods, 70 lines of code. The 3 biggest methods have 23 lines of combined unique lnes, and the same amount of duplicated code (try catch stuff). That is WET code. And we all know what happens when you leave things wet for to long, fungus, smell, dead kittens.

Note: Everything I wrote here is meant to help you in your journey in the land of code. If you feel offended, I apollogise. That was not what I meant to do. If you don't aggree, good for you. At the end of the day this is just another review. Happy coding!