patternphpMinor
My simple PDO wrapper class
Viewed 0 times
wrappersimpleclasspdo
Problem
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
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());
}
}
}
}Solution
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.
-
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.
-
Stay consistent with return types. What does your method return? Is it a resultset? A
-
Don't catch the
-
Don't
-
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!)Code Snippets
public function __construct($dsn) {Context
StackExchange Code Review Q#52414, answer score: 4
Revisions (0)
No revisions yet.