8
\$\begingroup\$

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

}
}
\$\endgroup\$
7
\$\begingroup\$

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 :)
\$\endgroup\$
1
  • \$\begingroup\$ Your style recommendations are applicable to coding in a great many, perhaps even most, other programming languages as well. \$\endgroup\$ – martineau Sep 16 '14 at 23:36
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ Not liking singletons without explaining why isn't quite informative. Also, a singleton can implement interfaces (or extend base classes), allows for reuse of the object (by reference to the instance) and is treated and passed around as a normal object. A static interface allows access to only the static methods. \$\endgroup\$ – jsanc623 Sep 16 '14 at 16:18
  • 1
    \$\begingroup\$ The question implied knowledge about the pros and cons of singletons. \$\endgroup\$ – Carl Sep 16 '14 at 16:26
  • 5
    \$\begingroup\$ But your answer came across as: on a personal level, you don't like singletons - without explaining why you don't like singletons. \$\endgroup\$ – jsanc623 Sep 16 '14 at 16:37
  • \$\begingroup\$ With the sample code, would the existing connection be reused or another connection opened for each query? \$\endgroup\$ – Kami Sep 16 '14 at 16:54
  • 1
    \$\begingroup\$ @jsanc623, it is personal and I get what you are syaing. I'm trying to think of a better way to open the post, but I'm not sure that the post is even on point anymore. \$\endgroup\$ – Carl Sep 16 '14 at 17:02

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.