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;
}
}
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, 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();
}
}
}
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;
}
}
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';
}
}
}
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();
}
}
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);
}
}
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;
}
$sql = "UPDATE IGNORE votes SET intTrackID = ? WHERE intTrackID = ?; DELETE FROM votes WHERE intTrackID = ?";
$query = $db->prepare($sql);
$query->execute(array($intNewTrackID, $intOldTrackID, $intOldTrackID));
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.