Please note – Apache have released a fix to this issue, and as such the below guidance has now been superseded by their fix.

I’ve been writing two open source projects over the last couple of years. My code has never really been particularly great, but I’ve been trying to learn how to improve my code and over the last few months, I’ve been really trying to polish up my coding skills.

A few months back, I attended a series of fantastic sessions at PHPNW about using Unit Testing, PHP CodeSniffer and phpDocumentor, and how these can be incorporated into Object Orientated code (or in fact, requiring Object Orientated code to implement them).

So, I went back into my main projects and started to look at how I could fix the code to start adopting these tools.

So, the first thing I needed to do was to start thinking about the structure. CCHits.net (which is the “big project” I’ve been working on recently) has several chunks of data, and these are:

User
Track
Artist
Show
ShowTrack
Vote
Chart

Each of these have been broken down into three “things” – The Object itself, a “Broker” which finds all the relevant objects, and a class to create new items, so let’s start with a user class. We’ll define a few properties in the initial class creation.

class UserObject
{
    protected $intUserID = 0;
    protected $strOpenID = "";
    protected $strCookieID = "";
    protected $sha1Pass = "";
    protected $isAuthorized = 0;
    protected $isUploader = 0;
    protected $isAdmin = 0;
    protected $datLastSeen = "";
}

By setting these as protected, it stops me from directly setting or accessing these variables from outside of the class – instead I want to do it from a function, so let’s add those in (I’ll just do one – assume these will be copied on to the rest of the values).

class UserObject
{
    protected $strOpenID = "";

    function set_strOpenID($strOpenID = "") {
        if ($strOpenID != $this->strOpenID) {
            $this->strOpenID = $strOpenID;
        }
    }

    function get_strOpenID() {
        return $this->strOpenID;
    }
}

In the set_ functions, we already do a little bit of error checking here (is the value already set to this?) but we could add other things like, on the integer items, is it actually an integer, with the boolean values ($is[SOMETHING]), make sure it’s set to 1 or 0 (or true/false).

Now, let’s add some documentation to this:

/**
 * CCHits.net is a website designed to promote Creative Commons Music,
 * the artists who produce it and anyone or anywhere that plays it.
 * These files are used to generate the site.
 *
 * PHP version 5
 *
 * @category Default
 * @package  CCHitsClass
 * @author   Jon Spriggs 
 * @license  http://www.gnu.org/licenses/agpl.html AGPLv3
 * @link     http://cchits.net Actual web service
 * @link     http://code.cchits.net Developers Web Site
 * @link     http://gitorious.net/cchits-net Version Control Service
 */
/**
 * This class deals with user objects
 *
 * @category Default
 * @package  Objects
 * @author   Jon Spriggs 
 * @license  http://www.gnu.org/licenses/agpl.html AGPLv3
 * @link     http://cchits.net Actual web service
 * @link     http://code.cchits.net Developers Web Site
 * @link     http://gitorious.net/cchits-net Version Control Service
 */
class UserObject
{
    /**
     * This is the Setter function for the strOpenID value
     *
     * @param string $strOpenID The value to set
     *
     * @return void There is no response from this function
     */
    function set_strOpenID($strOpenID = "") {
        // Do stuff
    }
}

So, let’s make it do stuff with the database. Firstly, we need to set up a connection to the database. I’ll be using PDO (PHP Database Object, I think) to set up the connection (I’ll show why in a minute).

So, here’s another class – Database, this time it’s using a singleton factory (which is to say, while it may exist many times in the code, it’ll only ever have one connection open at once) – note it’s got hard-coded authentication details here – this isn’t how it actually is in my code, but this way it’s a bit more understandable!

class Database
{
    protected static $handler = null;
    protected $db = null;

    /**
     * This function creates or returns an instance of this class.
     *
     * @return object $handler The Handler object
     */
    private static function getHandler()
    {
        if (self::$handler == null) {
            self::$handler = new self();
        }
        return self::$handler;
    }

    /**
     * This creates or returns the database object - depending on RO/RW requirements.
     *
     * @return object A PDO instance for the query.
     */
    public function getConnection()
    {
        $self = self::getHandler();
        try {
            $self->rw_db = new PDO('mysql:host=localhost;dbname=cchits', 'cchits', 'cchits', array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
            return $self->rw_db;
        } catch (Exception $e) {
            echo "Error connecting: " . $e->getMessage();
            die();
        }
    }
}

So, now let’s create yet another class, called GenericObject. This will be re-used in all of the “Object” classes to perform our main database calls. Try to move as much code up to your highest level objects – so you could have a “validateBoolean($value)” function that is used any time you set or get a boolean value here… we even mentioned something like this above!

class GenericObject
{
    protected $arrDBItems = array();
    protected $strDBTable = "";
    protected $strDBKeyCol = "";
    protected $arrChanges = array();

    /**
     * Commit any changes to the database
     *
     * @return boolean Status of the write action
     */
    function write()
    {
        if (count($this->arrChanges) > 0) {
            $sql = '';
            $strDBKeyCol = $this->strDBKeyCol;
            $values[$strDBKeyCol] = $this->$strDBKeyCol;
            $values = array();
            foreach ($this->arrChanges as $change) {
                if ($sql != '') {
                    $sql .= ", ";
                }
                if (isset($this->arrDBItems[$change])) {
                    $sql .= "$change = :$change";
                    $values[$change] = $this->$change;
                }
            }
            $full_sql = "UPDATE {$this->strDBTable} SET $sql WHERE {$this->strDBKeyCol} = :{$this->strDBKeyCol}";
            try {
                $db = Database::getConnection();
                $query = $db->prepare($full_sql);
                $query->execute($values);
                return true;
            } catch(Exception $e) {
                return false;
            }
        }
    }

    /**
     * Create the object
     *
     * @return boolean status of the create operation
     */
    protected function create()
    {
        $keys = '';
        $key_place = '';
        foreach ($this->arrDBItems as $field_name=>$dummy) {
            if ($keys != '') {
                $keys .= ', ';
                $key_place .= ', ';
            }
            $keys .= $field_name;
            $key_place .= ":$field_name";
            $values[$field_name] = $this->$field_name;
        }
        $full_sql = "INSERT INTO {$this->strDBTable} ($keys) VALUES ($key_place)";
        try {
            $db = Database::getConnection();
            $query = $db->prepare($full_sql);
            $query->execute($values);
            if ($this->strDBKeyCol != '') {
                $key = $this->strDBKeyCol;
                $this->$key = $query->lastInsertId();
            }
            return true;
        } catch(Exception $e) {
            return false;
        }
    }

    /**
     * Return an array of the collected or created data.
     *
     * @return array A mixed array of these items
     */
    function getSelf()
    {
        if ($this->strDBKeyCol != '') {
            $key = $this->strDBKeyCol;
            $return[$key] = $this->$key;
        }
        foreach ($this->arrDBItems as $key=>$dummy) {
            $return[$key] = $this->$key;
        }
        return $return;
    }
}

Any protected functions or variables are only accessible to the class itself or classes which have been extended from it (referred to as a child class). We need to make some changes to our UserObject to make use of these new functions:

class UserObject extends GenericObject
{
    // Inherited Properties
    protected $arrDBItems = array(
        'strOpenID'=>true,
        'strCookieID'=>true,
        'sha1Pass'=>true,
        'isAuthorized'=>true,
        'isUploader'=>true,
        'isAdmin'=>true,
        'datLastSeen'=>true
    );
    protected $strDBTable = "users";
    protected $strDBKeyCol = "intUserID";
    // Local Properties
    protected $intUserID = 0;
    protected $strOpenID = "";
    protected $strCookieID = "";
    protected $sha1Pass = "";
    protected $isAuthorized = 0;
    protected $isUploader = 0;
    protected $isAdmin = 0;
    protected $datLastSeen = "";

    function set_strOpenID($strOpenID = "") {
        if ($this->strOpenID != $strOpenID) {
            $this->strOpenID = $strOpenID;
            $this->arrChanges[] = 'strOpenID';
        }
    }
}

Notice I’ve stripped the “phpDoc” style comments from this re-iteration to save some space! In the real code, they still exist.

Now we have an object we can work with, let’s extend it further. It’s probably not best practice, but I find it much more convenient to create new objects by extending the UserObject into a new class called NewUserObject. Notice once we’ve set our database items, we run the create(); function, which was previously defined in the GenericObject class.

class NewUserObject extends UserObject
{
    public function __construct($data = "")
    {
        if (strpos($data, "http://") !== false or strpos($data, "https://") !== false) {
            $this->set_strOpenID($data);
        } elseif ($data != "") {
            $this->set_sha1Pass($data);
        } else {
            if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
                $cookie_string = $_SERVER['HTTP_X_FORWARDED_FOR'];
            } else {
                $cookie_string = $_SERVER['REMOTE_ADDR'];
            }
            $cookie_string .= $_SERVER['HTTP_USER_AGENT'];
            $cookie_string .= $_SERVER['HTTP_ACCEPT_LANGUAGE'];
            $cookie_string .= $_SERVER['HTTP_ACCEPT_ENCODING'];
            $cookie_string .= $_SERVER['HTTP_ACCEPT_CHARSET'];
            $this->set_strCookieID(sha1sum($cookie_string));
        }
        $this->datLastSeen = date("Y-m-d H:i:s");
        $_SESSION['cookie'] = sha1($cookie_string);
        return $this->create();
    }
}

Using the code $user = new NewUserObject($login_string); we can create a new user, but how about retrieving it.

This is where the reason I’m loving PDO comes into play. See, before PDO, when you did a database request, you might have had something like this:

$db = mysql_connect("localhost", "root", "");
if ($db == false) {
    die ("Failed to connect to the Database Server");
}
if (! mysql_select_db("database")) {
    die ("Failed to select the database");
}
$intUserID = mysql_real_escape_string($_GET['intUserID']);
$sql = "SELECT * FROM users WHERE intUserID = '$intUserID' LIMIT 1";
$qry = mysql_query($sql);
if (mysql_errno() > 0) {
    echo "Failed to make Database Call: " . mysql_error();
} else {
    if (mysql_num_rows($qry) == 0) {
        echo "Failed to retrieve record 1";
    } else {
        $row = mysql_fetch_array($qry);
    }
}

Now, let’s assume you’re looking for a few users? You need to do more of those mysql_real_escape_strings and mysql_num_rows() and mysql_fetch_array()s to get your data out – it’s far from clean and clear code.

How about this instead?

try {
    $db = Database::getConnection();
    $sql = "SELECT * FROM users WHERE intUserID = ? LIMIT 1";
    $query = $db->prepare($sql);
    $query->execute(array($_GET['intUserID']));
    $row = $query->fetch();
} catch(Exception $e) {
    echo $e;
    $row = false;
}

The most complicated bit here is that you’re having to prepare your SQL query and then tell it what to get. The reason we do that is that PDO will automatically ensure that anything being passed to it using the ? is sanitized before passing it into the query. If you look back at the GenericObject class we created earlier, it uses something like this there too, except there (if you work out what it’s doing) it prepares something like INSERT INTO users (intUserID) VALUES (:intUserID); and then executes it with the values like this: array(‘:intUserID’=>1) and with this you can have some very complex statements. Other code you might spot while mooching through CCHits (if you decide to) looks like this:

$sql = "UPDATE IGNORE votes SET intTrackID = ? WHERE intTrackID = ?; DELETE FROM votes WHERE intTrackID = ?";
$query = $db->prepare($sql);
$query->execute(array($intNewTrackID, $intOldTrackID, $intOldTrackID));

By wrapping it all up in a try/catch block, you can get your error dumped in one place, including a stack trace (showing where the issue turned up). You don’t need to check for mysql_num_rows – if the row doesn’t exist, it’ll just return a false. Sweet.

Where it gets REALLY nice, is that if you swap $row = $query->fetch() with $object = $query->fetchObject(‘UserObject’); and it’ll create the object for you!

So, now, with the function getUser (visible at https://gitorious.org/cchits-net/website-rewrite/blobs/master/CLASSES/class_UserBroker.php) it’ll try to return a UserObject based on whether they’re using OpenID, Username/Password or just browsing with a cookie… and if it can’t, it’ll create you a new user (based on the above criteria) and then return you that UserObject.

The last thing to add, is that I wrapped up all the nice phpDocumentor, PHP CodeSniffer functions, plus wrote a script to check for missing or incorrectly labelled functions across a suite of classes. These sit in https://gitorious.org/cchits-net/website-rewrite/trees/master/TESTS if you want to take a look around :)

EDIT 2011-08-25: Correcting some errors in the code, and to adjust formatting slightly.
EDIT 2012-05-05: Changed category, removed the duplicated title in the top line, removed some whitespace.