Is this wrapper for PDO 'good code' ? Are there any potential problems?

Question

I built this class to work with PDO, to make SQL queries 'easier' and less to worry about.

Here are my thoughts

• Should it be more like class DB extends PDO?
• Is the query method too big? Should it be split into private methods which are called.. is this what is known as loose coupling?
• Is my way for detecting a SELECT query too ugly for it's own good?
• What other problems are evident? As I am sort of learning-as-I-go, I'm sure I could have overlooked a lot of potential problems.

Thank you

`

 class Db
 {
    private static $_instance = NULL;


    private function __construct() {

        // can not call me
    }

    private function __clone() {

        // no!
    }

    public static function getInstance() {

        if (!self::$_instance)
        {

            try {

                self::$_instance = new PDO('mysql:host=' . CONFIG_MYSQL_SERVER . ';dbname=' . CONFIG_MYSQL_DATABASE, CONFIG_MYSQL_USERNAME, CONFIG_MYSQL_PASSWORD);;
                self::$_instance-> setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

            } catch(PDOException $e) {

                trigger_error($e->getMessage());

            }

        }

        return self::$_instance;


    }



    public static function query($query /*string*/, $bindings = NULL)
    {

        $queryPortion = substr($query,0, 6);

        try {

            if ($bindings) {

                    $prepared = self::getInstance()->prepare($query);

                    foreach($bindings as $binding=>$data) { // defaults to string

                        if (!is_array($data)) {
                            $prepared->bindParam($binding, $data); 

                        } else {

                            switch(count($data)) {

                                case 1:
                                    $prepared->bindParam($binding, $data['value']);
                                    break;                          

                                case 2:
                                    $prepared->bindParam($binding, $data['value'], $data['dataType']);
                                    break;

                                case 3:
                                    $prepared->bindParam($binding, $data['value'], $data['dataType'], (int)$data['length']);
                                    break;                          

                                default:
                                    trigger_error('An error has occured with the prepared statement bindings.');
                                    return false;
                                    break;
                            }
                        }

                    }

                    $prepared->execute();

                    return $prepared->fetchAll(PDO::FETCH_ASSOC);


            } else if (String::match($queryPortion, 'select')) { // if this is a select query

                $rows = self::getInstance()->query($query);

                return $rows->fetchAll(PDO::FETCH_ASSOC);

            } else {

                return self::getInstance()->exec($query);

            }


        } 
        catch(PDOException $e)
        {
            trigger_error($e->getMessage());
        }


    }

    public static function getLastInsertId()
    {
        try {
            self::getInstance()->lastInsertId();
          }
        catch(PDOException $e)
        {
            trigger_error($e->getMessage());
        }

    }

    public static function disconnect()
    {
        // kill PDO object
        self::$_instance = NULL;

    }
 }

Answer 1

It's not bad and as it's been said it might help for small applications although it's mostly a very thin abstraction on another abstraction. It's not bringing a lot of others functionalities.

Something you might want to consider, amongst other things:

• As this is PHP5 code, use exceptions instead of trigger_error and set_exception_handler if necessary until exceptions are more widespread, but it's definitely cleaner and more future-proof.
• You are using a singleton, it's not a bad thing necessarily but in this case, for example, one shortcoming will be that you'll only be able to handle one connection to one database.
• I don't know if you make use of stored procedures, but a stored procedure might return a result set through the query() method too.
• You have two semi-colons (;;) at the end of your new PDO line.

That being said, I don't think your query method is too big and there's not much that could be recalled from elsewhere in there at the moment. Though as soon as you see two or three lines that could be called from another function, split it. That's a good way to DRY.

Answer 2

Yes and No.

It is good code for a simple quick and dirty application.

The problem comes when you use this in a more complex structured application. Where the error handling will vary depending on which sql you are executing.

Also any severe errors will show up as "problem at line 999" type errors where 999 is in your super duper routine and you will have difficulty tracing it back to a particular sql request.

Having said that I do this sort of thing myself all the time on small projects.

Answer 3

Here's what I've used (just replace the references to Zzz_Config with $GLOBALS['db_conf'] or something):

/**
 * Extended PDO with databse connection (instance) storage by name.
 */
class Zzz_Db extends PDO
{
    /**
     * Named connection instances.
     *
     * @var array
     */
    static private $_instances;

    /**
     * Retrieves (or instantiates) a connection by name.
     *
     * @param  string $name  Connection name (config item key).
     * @return Zzz_Db        Named connection.
     */
    static public function getInstance($name = null)
    {
        $name = $name === null ? 'db' : "db.$name";
        if (!isset(self::$_instances[$name])) {
            if (!$config = Zzz_Config::get($name)) {
                throw new RuntimeException("No such database config item: $name");
            }
            if (!isset($config['dsn'])) {
                if (!isset($config['database'])) {
                    throw new RuntimeException('Invalid db config');
                }
                $config['dsn'] = sprintf('%s:host=%s;dbname=%s',
                    isset($config['adapter']) ? $config['adapter'] : 'mysql',
                    isset($config['host']) ? $config['host'] : 'localhost',
                    $config['database']);
            }

            $db = self::$_instances[$name] = new self(
                $config['dsn'],
                isset($config['username']) ? $config['username'] : null,
                isset($config['password']) ? $config['password'] : null);
            $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            //$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, 'Zzz_Db_Statement');

            if ($db->getAttribute(PDO::ATTR_DRIVER_NAME) == 'mysql') {
                $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
                $db->exec('SET CHARACTER SET utf8');
            }
        }

        return self::$_instances[$name];
    }
}

Usage whould be:

$db = Zzz_Db::getInstance(); // or Zzz_Db::getInstance('some_named_db')
$stmt = $db->prepare('SELECT ...

The goal is to keep the db configuration in an *.ini file (editable by a non-coder).

Answer 4

I went the other way and made a class that extends PDO with a bunch of wrapper functions around prepare()/execute(), and it's much nicer than the built in functions (though that's a bit subjective...).

One other thing: you should set PDO::ATTR_EMULATE_PREPARES to false unless you're using a really old version of mysql (<=4.0). It defaults to true, which is a huge headache and causes things to break in obscure ways... which I'm guessing is the reason you've got a huge wrapper around bindParam() in the first place.

Answer 5

To answer your question, if it is a good code or not, ask yourself:
What is the added value of my code compared to using PDO directly?

If you find a good answer, go for using your code. If not, I would stick with PDO.

Also try considering implementing Zend Framework's DB class which works on its own and supports PDO.