Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've built a Connection class that uses a PDO connection, but it looks messy and im sure there are ways of improving it. Currently I only want 1 instance of the connection (singleton).

<?php
namespace AQEConnect {
    use \PDO as PDO;

    /** Includes */
    require_once dirname(__DIR__) . '/config.php'; // Contains the database config (HOST, DBNAME, USERNAME, PASSWORD).

    /**
     * A static class that holds the database connection.
     */
    class Connection
    {
        /**
         * @var null|PDO Contains the PDO connection to the database.
         */
        private static $_connection = null;

        /**
         * Initialises a connection to the database if the connection is null. If PDOException is thrown then
         * it dies.
         */
        private static function _connect()
        {
            $connection_attempt_limit = 5; // Maximum number of attempts to connect to Database.
            for ($con_attempt_num = 0; $con_attempt_num <= $connection_attempt_limit; $con_attempt_num++)
                try {
                    if (self::$_connection === null) { // Check if connection is currently null.
                        self::$_connection = new PDO('mysql:host=' . HOST . ';port=' . PORT . ';dbname=' . DBNAME, USERNAME,
                            PASSWORD, array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'));
                        self::$_connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_SILENT);
                    }
                } catch (PDOException $exception) {
                    /** Error occurred so roll the connection back and set it to null. */
                    self::$_connection->rollBack();
                    self::$_connection = null;

                    /** If connection attempt number is at the maximum number of attempts then trigger an 'E_CORE_ERROR'
                     * (and log it).  */
                    if ($con_attempt_num == $connection_attempt_limit) {
                        $trace = debug_backtrace();
                        trigger_error('Exception: [' . $exception . '] in ' . $trace[0]['file'] .
                            ' on line ' . $trace[0]['line'], E_ERROR);
                        die; // If PDOException is thrown then die.
                    }
                }
        }

        /**
         * @return bool Whether the class currently has a connection.
         */
        public static function hasConnection()
        {
            return self::$_connection !== null;
        }

        /**
         * @return null|PDO Current connection.
         */
        public static function get()
        {
            if (self::$_connection === null) self::_connect(); // If connection is currently null then connect.

            return self::$_connection;
        }

        /**
         * Stop Connection class from being cloned, as there is currently no
         * legitimate reason why it should ever need to be cloned.
         */
        protected function __clone()
        {
        }

        /**
         * @return string Current Connection class details.
         */
        public function __toString()
        {
            return self::hasConnection() ? 'Connected to: ' . DBNAME . '.' : 'No Connection.';
        }
    }
}
share|improve this question
1  
I plan on writing a proper response at some point (probably Friday), but for the time being, you might should read stackoverflow.com/questions/4595964/who-needs-singletons/…. That's actually a very brief explanation of why singletons and PHP don't mix. I've yet to see a legitimate use of a singleton in PHP in the wild. Potentially also relevant: stackoverflow.com/questions/228164/…. In short, what I'm trying to say is that a class modeling a DB connection should never be a singleton. –  Corbin Dec 5 '12 at 23:12

2 Answers 2

Basic mistakes

  • Forget static, it's not object oriented, it's ugly and unreadable

  • Forget constans in classes, inject them instead (global hyperspace is bad, untestable, uncontrolable)

Generic approach

<?php

interface ICredentials {

    function UserID();

    function Password();
}

interface IDbProvider {

    /**
     * 
     * @return array Driver specific options for PDO
     */
    function GetOptions();
}

class MySQL implements IDbProvider {

    public function GetOptions() {
        return array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8');
    }

}

/*abstract*/ class SqlConnection {

    /**
     *
     * @var \PDO
     */
    private $_connection;
    private $_connectionString;

    /**
     *
     * @var \ICredentials 
     */
    private $_credentials;

    /**
     *
     * @var \IDbProvider
     */
    private $_options;

    public function __construct($connectionString, \ICredentials $credentials, \IDbProvider $options) { //or protected, especially if abstract class
        $this->_connectionString = $connectionString;
        $this->_credentials = $credentials;
        $this->_options = $options;
    }

    /**
     * 
     * @return \PDO
     */
    public function Connection() { //or protected, especially if abstract class
        if ($this->_connection == NULL) {
            $options = $this->_options->GetOptions();
            $options[\PDO::ATTR_ERRMODE] = \PDO::ERRMODE_EXCEPTION;

            $this->_connection = new \PDO($this->_connectionString, $this->_credentials->UserID(), $this->_credentials->Password(), $options);
        }

        return $this->_connection;
    }

    protected function _close() {
        if ($this->_connection != NULL) {
            $this->_connection = NULL;
        }
    }

    public function __destruct() {
        $this->_close();
    }

}

Usage #1

(First you have to implement IDbCredentials.)

$connection = new SqlConnection('mysql:host=localhost;dbname=testdb', $credentialsInstance, new MySQL());

Usage #2 as abstract

The instantiating is the same, in this current example the options aren't injected (i would do this in a generated db specific object context class but in a general DbConnection as in the example).

class DbConnection extends SqlConnection {

    public function __construct($connectionString, \ICredentials $credentials) {
        parent::__construct($connectionString, $credentials, new MySQL());
    }

    public function Execute($something /* ... parameters ... */) {
        try {
            $this->Connection()->beginTransaction();

            /* something */

            $this->Connection()->commit();
        } catch (\PDOException $pdoExc) {
            if ($this->Connection() != NULL && $this->Connection()->inTransaction()) {
                $this->Connection()->rollBack();
            }

            throw $pdoExc;
        }
    }

}

$connection = new DbConnection('mysql:host=localhost;dbname=testdb', $credentialsInstance);

$connection-Execute("INSERT INTO ...", array(/* params */));
  • Easy to read
  • Easy to understand
  • Easy to test
  • Easy to reuse
  • Maybe __clone can be implemented :)

Edit

You can create a derived class which constructor accepts only one object instance which is holding all the information what are needed to establish the connection. (The implementation can be read the configuration values from XML or any other source.)

share|improve this answer
    
I like the format of that and is defiantly very reusable, but can you give me an example of how you would implement it (an example of opening a new connection). Thanks –  Adam Dec 4 '12 at 19:46
1  
I have updated my answer. –  Peter Kiss Dec 4 '12 at 22:24

This is a little bit of a reworked class based on your code example. Some things to keep in mind while writing your version:

  • Is this testable? Currently it is not; the db config is assumed, the toString is assuming a db credential ( DBNAME ), etc.

  • Your db connectivity is coupled to this class. What would happen if you wanted to use this for another application?

  • Your db connectivity is hinged on the fact that the assumption is made there is a config file. What will happen if there is no config.php? The path to the config is hardcoded to be the root of this DIR. What happens if this script is moved?

  • Declaring database connection parameters as global definitions is generally not a good idea. Having PORT/HOST/DBNAME/USERNAME/PASSWORD out in the global space ( assuming you are defining them in the config.php ) is a security risk.

Anyhow, here is a slighly reworked class.

namespace AQEConnect;

class Connection
{
/**
 * @var \PDO|null
 */
private static $connection;

/**
 * Return whether or not there is a current database connection
 *  or not.
 * 
 * @return string
 */
public function __toString()
{
    if (!is_defined('DBNAME')) {
        self::extractConfiguration();
    }

    return (!($connection instanceof \PDO)) ? 
        'No Connection' : 'Connected to: ' . DBNAME;
}

/**
 * Based on a config static location, extract the necessary database
 *  configuration from the required file.
 */
public static function extractConfiguration()
{
    // Include config to local database credentials for connecting
    $configPath = dirname(__DIR__) . '/config.php';
    if (!file_exists($configPath)) {
        trigger_error('Exception: Failed to find database connection configuration.');
        die;
    }

    require_once dirname(__DIR__) . '/config.php';
}

/**
 * Connects to the database through the PDO abstraction layer
 *  given a set of assumed database credentials. It will attempt
 *  to connect 5 times before it fails.
 *
 * @return boolean
 */
public static function connect()
{
    // Before we attempt any of this logic make sure that we do
    //  not currently have a connection.
    if (self::$connection) {
        return true;
    }

    // Make sure the config credentials are loaded.
    self::extractConfiguration();

    $attemptsLimit = 5;
    $attempt       = 0;
    while ($attempt < $attemptsLimit) {
        self::$connection = new PDO(
            'mysql:host=' . HOST . ';port=' . PORT . ';dbname=' . DBNAME, 
            USERNAME, 
            PASSWORD, 
            array(
                PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'
            )
        );

        if (self::$connection) {
            break;
        }

        $attempt++;
    }

    // If we failed to get a connection at this point we want to go ahead
    //  and kill the script and throw an error.
    if (!self::$connection) {
        $trace = debug_backtrace();
        trigger_error(
            'Exception: [' . $exception . '] in ' . $trace[0]['file'] .
                ' on line ' . $trace[0]['line'], E_ERROR
        );
        die; 
    }

    return true;
}

/**
 * Returns a boolean result which represents whether or not
 *  an open connection is currently present.
 *
 * @return boolean
 */
public static function hasConnection()
{
    return is_null(self::$connection);
}

/**
 * Returns an instance of the set database instance. If the
 *  connection is null, it will attempt to connect.
 * 
 * @return \PDO
 */
public static function getInstance()
{
    if (!self::$connection) {
        self::connect();
    }

    return self::$connection;
}
}
share|improve this answer
1  
I just started to -1 this since the code in it is worse than what the question contains now, but then I realized that the question has been revised :). Even relative to the original code in the question though, this answer still has some major flaws. In particular, extractConfiguration cannot be called twice without issuing warnings, and constants are always global, even if defined in a method so your extractConfiguration is pointless anyway. (Edit: Actually, I didn't realize the question had been edited until it was too late to undo the downvote. Sorry.) –  Corbin Dec 5 '12 at 7:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.