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.