My simple PDO wrapper class

Question

I made a simple database class and I wanted to know if there are any improvements I could work on as far as readability, efficiency, methods and making it modular goes.

Any other suggestions are also welcome!

Database.php

class Database
{
 private $config;
 private $pdo;
 public function __construct(Config $config)
 {
 $this->config = $config;
 }
 public function query($sql, $params = null)
 {
 $this->connect();
 $sth = $this->pdo->prepare($sql);
 $params = is_array($params) ? $params : is_null($params) ? null : [$params];
 if ($params && $sth->execute($params)) {
 return $sth;
 } elseif ($sth->execute()) {
 return $sth;
 }
 return false;
 }
 private function connect()
 {
 if (!$this->pdo) {
 try {
 $dsn = 'mysql:host=' . $this->config->get('mysql/host') . ';dbname=' . $this->config->get('mysql/database') . ';charset=' . $this->config->get('mysql/charset');
 $this->pdo = new PDO($dsn, $config->get('mysql/username'), $config->get('mysql/password'));
 $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
 $this->pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_OBJ);
 } catch (Exception $e) {
 die($e->getMessage());
 }
 }
 }
}

• Pinoniq: What you did here is take a pretty good class (PDO) and only allow one method to be called, the query(); ieuk
• Kid Diamond: The query method returns the statement handler on which you can further use the PDO methods like fetch, fetchAll, rowCount, etc.
• Pinoniq: And what about transactions? and rollback? or binding a variable to a query and executing it multiple time? The entire fun of pdo prepared statemenst (using the same statement over and over again by binding params) is not available now
• Kid Diamond: I guess so. I'm new to PDO and this is my first PDO wrapper class and I have just made it based on what my application requires at this moment. And I'll add extra methods to the class as I need them.
• Pinoniq: thats a good approach :)

Answer 1

I see several things up for improvements:

• What problem are you solving? PDO on itself is an abstraction. Your abstraction over it is, on most cases, redundant.

  • A abstraction is a Mapper for example, which uses the PDO connection to map an object to the database (or whatever form of storage). See The DataMapper pattern

• Registry is an antipattern. Why does your database class need all of the config? Just for the 4 details it needs? Just pass those in to the constructor/connect method.

  public function __construct($dsn) {
  

• Stay consistent with return types. What does your method return? Is it a resultset? A PDOStatement object? A boolean? Pick one and stick with it. Utilize Exceptions for errors.

• Don't catch the (PDO)Exception inside the method. You don't know what you want to do yet! Maybe I want to try to connect, and fallback to files if it fails. Maybe I want to log? Maybe I want to send an email? Your Database object doesn't know the context in which it's called, and so it doesn't know the context in which to deal with errors.

• Don't die(). When you die, the entire script dies, just like that. That can be very undesirable in cases where you instantiate a database connection in the middle of the application and it fails. Allow the exception to bubble up, breaking execution when needed, until it's being handled. (And if it isn't, it'll fatal the script anyway!)

• Madara's Ghost: That's not all, but that's all my time for now. Expect edits!
• Kid Diamond: I like the concept of the data mapper pattern. It basically lets me get rid of my entire database class and implement mappers instead to connect specific objects to the database, if I understood that correctly. And yes, I just realized that it's not necessary to inject the entire config, so I will be injecting only the needed arguments instead. If you can give me a data mapper code example, that would be great and how to connect Object <- Mapper -> DB.
• Madara's Ghost: The idea of an abstraction which is the basis of OOP, is to make layer not be aware of lower layers. Just like when you write PHP code you aren't concered about the underlying C code, your Objects shouldn't be aware of the storage they are kept in. That's the job of the Mapper. This allows for reusability, because if done right, you now get an object which is independant from database, and can be reused in environments where the data is kept elsewhere (You only need to implement new mappers)
• Madara's Ghost: @KidDiamond: I edited with a couple more bullets, check it out now.
• Kid Diamond: Thanks you. I will be getting rid of this class and implement data mappers instead.