Singleton Database wrapper

Question

Before everybody yells at me about why I wrote a PDO wrapper class, it's to avoid writing try/catch, prepare(), execute(), etc. each time a query should be executed.

I have pretty big website, so instead of repeating:

$sth =  $dbh->prepare('Some query');
$dbh->BindParam('x', $x);
$dbh->BindParam('y', $y);
 //..etc
$sth->execute();

a few thousands times I can simply do:

$res = $dbh->query('Some query', $x1, $y1, //etc);
$res->fetch(\PDO::FETCH_OBJ)

And also because I want to do some operations on each query like count queries, execution time, etc.

Is this Singleton wrapper really bad or overkill?

namespace App\Database {


class Database
{

 static private $PDOObject;

 function __construct() 
 {
     if(!self::$PDOObject) {

        try{

            self::$PDOObject = new \PDO('mysql:host=127.0.0.1;dbname=test', 'username', 'pentagonpassword');

        }
        catch(\PDOException $e) {
             //handle exception
             throw $e;
        }
        return self::$PDOObject;


      }
 }


 public function query($query, ...$params)
 {

   try
   {
    //start timer and other stuff (count queries etc)
    $sth = self::$PDOObject->prepare($query);
    $sth->execute($params);
    //end timer
    return $sth;
    }
    catch(\PDOException $e)
    {
       throw $e;
    }




 }

 public function lastid($name = NULL)
 {
   return self::$PDOObject->LastInsertId($name);
 }


  //Other methods that processes some complex stuff

}
}

Answer 1

Singleton

I wouldn't call your Database class a singleton wrapper, as you can have multiple instances of it (not of the PDOObject field, but of the Database class).

See here and here for actual singleton pattern.

I don't think that your approach is necessarily bad though.

Is this overkill?

I think that you are a bit dishonest in your example. Your not actually using BindParam in your code, and you would still have to catch the exception that is thrown. So instead of saving multiple lines per query as you suggest, you are actually only saving one, which isn't that much.

But doing it this way does provide you the ability to easily add functionality to executing a query, or change how you execute queries all together. I think that this is a big advantage and a good case for having this class.

Returning in the constructor

return self::$PDOObject;

Why are you returning the PDO instance in the constructor? This seems a little odd.

Style

I say this a lot, but good style actually is important and leads to easier to read code.

• remove all unnecessary newlines. You have a lot of newlines, which makes your code hard to read
• be consistent with your curly brackets and indentation (always indent the same amount, always have curly brackets close at the same indentation as the opening statement).
• also be consistent with spaces (try{ -> try {)
• comments: //handle exception isn't a useful comment, especially if it is handled by just throwing the exception :)

Answer 2

It's fine to have a wrapper to abstract PDO functionality, especially to avoid code duplication. The class itself looks ok to me as well - aside from some minor issues, as delineated in the comments in the code:

namespace App\Database;

class Database {
    static private $PDOObject;

    # ******************************************************
    # Put all the configuration stuff here, so that we don't have to delve into the code to modify strings
    # ******************************************************
    static private $host = '127.0.0.1';
    static private $db = 'test';
    static private $user = 'username';
    static private $pass = 'pentagonpassword'

    /**
     * Construct
     * 
     * @return object PDOObject
     * @throws PDOException
     */
    function __construct() {
        if(!self::$PDOObject) {
            try {
                self::$PDOObject = new \PDO('mysql:host=' . self::$host . ';dbname=' . self::$db, 
                                            self::$user, self::$pass);
            } catch(\PDOException $e) {
                throw $e;
            }
        }

        # ******************************************************
        # Move the return here, so that we return the object if it exists already as well
        # ******************************************************
        return self::$PDOObject;
    }

    /**
     * Query description
     * 
     * @return object $sth
     * @throws PDOException
     */
    public function query($query, ...$params) {
        try {
            # Start timer, count queries, etc
            $sth = self::$PDOObject->prepare($query);
            $sth->execute($params);
            # End timer

            return $sth;
        } catch(\PDOException $e) {
            throw $e;
        }
    }

    /**
     * lastid description
     *
     * @param $name NULL
     * @return integer
     */
    public function lastid($name = NULL) {
        return self::$PDOObject->LastInsertId($name);
    }
}

Additionally, pick up the habit of commenting your functions - you'll thank yourself later.

Answer 3

I'll just say it, I don't like singletons.

Rather than create static member variables, I would create static class functions:

class Database
{
  private function __construct()
  {
    # connect to the database, etc.
  }

  private function doQuery()
  {
    # actually preform the query
  }

  public static function query()
  {
    $d = new Database();
    $d->doQuery();
  }
}

Now you can have a static interface to your class and still be able to limit the scope of your variables.