PDO wrapper class for database

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.

```
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

This review will consist of 2 parts:

• The design/concept

• 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

StackExchange Code Review Q#69322, answer score: 2