Peer Review: Taking Code And Making It Better

On Wednesday, August 12th, we had a meeting of the DC PHP Developer’s Group. Keith Casey of Blue Parabola led a code review of a member-submitted sample. The review was informative, educational, and helpful. With the permission of that member, I’ve decided to write a series on the tools for reviewing code and re-factoring it.

The code sample (included below) isn’t perfect. It needs work, and we’ll be working on it over the next few articles. Along the way, we’ll talk about strategies for identifying weaknesses, candidates for refactoring, and methods for writing better code before you get to the review stage. We’ll also refactor this into something more usable, and end up with a finished product that’s better than when we started.

The knowledge gained is based off the DC PHP Developer’s Group, as well as many notes that I put together. The refactoring process requires a lot of thought, and so this series will contain six more entries, starting with the first one on Monday. But before we begin, here’s the code sample we’ll be working with:

<?php // class_Twitter.php - NPC TWITTER AUTO-FEED
error_reporting(E_ALL); 

// SEND AN AUTOMATED TWEET
// USAGE EXAMPLE
//   require_once('class_Twitter.php');
//   $status = 'Hello World';
//   $t      = new Twitter;
//   $t->tweet($status);
//   unset($t); 

// DO NOT RUN THIS SCRIPT STANDALONE (HAS PASSWORD)
if (count(get_included_files()) < 2) {
    header("HTTP/1.1 301 Moved Permanently"); header("Location: /"); exit;
} 

class Twitter
{
    private $ch;
    private $you;
    private $user;
    private $pass;
    private $test;
    private $host;
    private $done; // ALREADY TWEETED?
    public  $res; 

    public function __construct()
    {
        $this->user = "??? TWITTER USERNAME";
        $this->pass = "??? TWITTER PASSWORD";
        $this->you  = "??? YOUR EMAIL ADDRESS"; 

        $this->host = "http://twitter.com/";
        $this->done = FALSE; // DEFAULT - NOT ALREADY TWEETED
        $this->test = FALSE; // DEFAULT - THIS IS LIVE, NOT A TEST 

        $this->ch   = curl_init();
        curl_setopt($this->ch, CURLOPT_VERBOSE, 1);
        curl_setopt($this->ch, CURLOPT_RETURNTRANSFER, 1);
        curl_setopt($this->ch, CURLOPT_USERPWD, "$this->user:$this->pass");
        curl_setopt($this->ch, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1);
        curl_setopt($this->ch, CURLOPT_POST, 1);
    } 

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

// SET AN INDICATOR THAT THIS IS NOT A LIVE TWEET
    public function test()
    {
        $this->test = TRUE;
    } 

// DETERMINE IF THE MESSAGE HAS ALREADY BEEN SEND
    private function already_tweeted($message)
    {
        $text     = mysql_real_escape_string(trim($message));
        $date     = date('Y-m-d');
        $code     = md5($text . $date);
        $sql      = "SELECT id FROM twitterLog WHERE thash = \"$code\" ORDER BY id DESC LIMIT 1";
        if (!$res = mysql_query($sql)) die(mysql_error());
        $num      = mysql_num_rows($res);
        if ($num) return TRUE;
        $sql      = "INSERT INTO twitterLog (tdate, thash, tweet) VALUES ( \"$date\", \"$code\", \"$text \" )";
        if (!$res = mysql_query($sql)) die(mysql_error());
        return FALSE;
    } 

// POST A MESSAGE TO TWITTER
    public function tweet($message)
    {
        if(strlen($message) < 1) return FALSE;
        if ($this->already_tweeted($message)) $this->done = TRUE; 

// IF ONLY A TEST, JUST EMAIL THE INFORMATION - DO NOT TWEET
        if ($this->test)
        {
            $msg = '';
            if ($this->done) $msg .= "ALREADY DONE ";
            $msg .= "TWEET: $message";
            mail($this->you, 'What We Would Have Tweeted', $msg);
            return TRUE;
        } 

// DO NOT REPEAT YOURSELF
        if ($this->done) return TRUE; 

// STATUS UPDATE ON TWITTER
        $this->host .= "statuses/update.xml?status=".urlencode( stripslashes( urldecode($message )));
        curl_setopt($this->ch, CURLOPT_URL, $this->host);
        $xxx       = curl_exec($this->ch);
        $this->res = curl_getinfo($this->ch);
        if ($this->res['http_code'] == 0)   return TRUE;
        if ($this->res['http_code'] == 200) return TRUE;
        return FALSE;
    }
}
?>

The original work of Brandon Savage.

Related posts:

1. Peer Review: Managing Coding Standards
2. Peer Review: Looking Into Abstraction
3. Peer Review: You Have Not Because You Ask Not (Requests & Responses)
4. Peer Review: Improving The Business Logic
5. Peer Review: Testable Code And Architecture

There are currently no comments.