4
\$\begingroup\$

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());
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ What you did here is take a pretty good class (PDO) and only allow one method to be called, the query(); ieuk \$\endgroup\$ – Pinoniq Jun 4 '14 at 15:20
  • \$\begingroup\$ The query method returns the statement handler on which you can further use the PDO methods like fetch, fetchAll, rowCount, etc. \$\endgroup\$ – Kid Diamond Jun 4 '14 at 15:23
  • \$\begingroup\$ 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 \$\endgroup\$ – Pinoniq Jun 4 '14 at 16:36
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Kid Diamond Jun 4 '14 at 16:44
  • \$\begingroup\$ thats a good approach :) \$\endgroup\$ – Pinoniq Jun 4 '14 at 18:42
4
\$\begingroup\$

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!)

\$\endgroup\$
  • \$\begingroup\$ That's not all, but that's all my time for now. Expect edits! \$\endgroup\$ – Madara Uchiha Jun 10 '14 at 14:39
  • \$\begingroup\$ 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. \$\endgroup\$ – Kid Diamond Jun 10 '14 at 14:56
  • 3
    \$\begingroup\$ 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) \$\endgroup\$ – Madara Uchiha Jun 10 '14 at 15:00
  • \$\begingroup\$ @KidDiamond: I edited with a couple more bullets, check it out now. \$\endgroup\$ – Madara Uchiha Jun 10 '14 at 17:22
  • \$\begingroup\$ Thanks you. I will be getting rid of this class and implement data mappers instead. \$\endgroup\$ – Kid Diamond Jun 11 '14 at 5:48

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.