Sudheer (@bngsudheer) wrote at 8/17/2009 1:28 am:Nice article. The world will be so different if every PHP beginner takes coding standards serious.
| « My Essential WordPress Plugins | Peer Review: Taking Code And Making It Better » |
If you need a sample of the code, please visit here.
One of the first things I look for when I check out code is how is the code organized? Is it laid out well? Is it coded to a particular standard?
In our code sample, the first thing we should address is how does the code look. There are a number of suggestions I would make immediately. Let’s dive in.
There are no DocBlocks or clear coding standards.
No clear coding standard jumps out at you right away when you read this code. There’s a lack of consistency, but beyond that, code completion is hindered by a lack of standards. Also, there’s no DocBlocks, which would help improve the documentation of the code.
There are lots of coding standards out there: PEAR, WordPress, Drupal and Zend Framework all have a coding standard in place that you can adopt in your own code. I highly recommend it.
The first thing I might do is add some DocBlocks. It will help us understand the code better.
Examining The Properties
The next thing I want to examine is the list of properties. Each property is private, with the exclusion of the public $res property. The $res property is actually unnecessary (it isn’t called by more than one method). From a style standpoint, most private properties are prefixed with an underscore (“_”) to demonstrate that they are, in fact, private. Let’s go ahead and make this code change:
<?php
class Twitter
{
/**
* @var resource Curl connection
*/
private $_ch;
/**
* @_var string Email address for administrator/tester
*/
private $_you;
/**
* @var string The username for the Twitter account.
*/
private $_user;
/**
* @var string The password for the Twitter account.
*/
private $_pass;
/**
* @var bool The flag for test mode
*/
private $_test;
/**
* @var string The URL for the API call
*/
private $_host;
/**
* @var bool Flag for whether or not this tweet has been sent today
*/
private $_done;
/**
* @var mixed Response of the cURL queries
*/
public $res;
[/sourcecode]
We will, of course, need to make that change throughout the code.
But there's something else about the properties that we need to examine. Because the properties are private, this class cannot be extended. This might be an intentional architectural choice; however, it is more likely that the author just didn't consider the possibility of extension.
I prefer t make my properties protected. I also prefer to make ALL of my properties protected. I prefer to access properties through methods, rather than directly from outside the object. Also, since the $res property is unnecessary, we'll go ahead and drop it. Our properties list now looks like this:
[sourcecode language="php"]
class Twitter
{
/**
* @var resource Curl connection
*/
protected $ch;
/**
* @var string Email address for administrator/tester
*/
protected $you;
/**
* @var string The username for the Twitter account.
*/
protected $user;
/**
* @var string The password for the Twitter account.
*/
protected $pass;
/**
* @var bool The flag for test mode
*/
protected $test;
/**
* @var string The URL for the API call
*/
protected $host;
/**
* @var bool Flag for whether or not this tweet has been sent today
*/
protected $done;
[/sourcecode]
<strong>Conditionals</strong>
One thing I noticed about this code as well is that it has a number of conditionals that are not wrapped in curly braces. While this is syntactically correct, it creates some issues for debugging later, and I generally prefer not to rely on it.
Take this for example (from the code):
if ($this->already_tweeted($message)) $this->done = TRUE;
If we ever decide we need to debug by, say, outputting the message, and we change the code to this:
if ($this->already_tweeted($message))
var_dump($message);
$this->done = TRUE;
…then we always are marking the tweet as being sent previously. This is clearly not optimal.
Let’s go ahead and wrap all of the “if” statements in brackets (shown at the end of the entry).
Improving The Constructor
A class should be a blank blueprint of an object – that is, it should be infinitely configurable, based on the conditions and things you pass into it. In this object, the constructor is limiting. The password and username have to be hardcoded in. Let’s change that by allowing it to take arguments for the username and password.
/**
* Constructor.
* @param string $username The username of the Twitter user
* @param string $password The password of the Twitter user
* @param string $email Optional email address for testing
*/
public function __construct($username, $password, $email = null)
{
$this->user = $username;
$this->pass = $password;
$this->you = $email;
$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);
}
This change will allow the object to accept credentials and be unique, with the class being closer to the blueprint it should be.
We’ve made a lot of changes to the code at this point. Let’s take a look at all of the code now and review where we’ve been:
<?php // class_Twitter.php - NPC TWITTER AUTO-FEED
error_reporting(E_ALL);
// DO NOT RUN THIS SCRIPT STANDALONE (HAS PASSWORD)
if (count(get_included_files()) < 2) {
header("HTTP/1.1 301 Moved Permanently"); header("Location: /"); exit;
}
/**
* Twitter Class
* @author Anonymous
*
*/
class Twitter
{
/**
* @var resource Curl connection
*/
protected $ch;
/**
* @var string Email address for administrator/tester
*/
protected $you;
/**
* @var string The username for the Twitter account.
*/
protected $user;
/**
* @var string The password for the Twitter account.
*/
protected $pass;
/**
* @var bool The flag for test mode
*/
protected $test;
/**
* @var string The URL for the API call
*/
protected $host;
/**
* @var bool Flag for whether or not this tweet has been sent today
*/
protected $done;
/**
* Constructor.
* @param string $username The username of the Twitter user
* @param string $password The password of the Twitter user
* @param string $email Optional email address for testing
* @return void
*/
public function __construct($username, $password, $email = null)
{
$this->user = $username;
$this->pass = $password;
$this->you = $email;
$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);
}
/**
* A destructor.
* @return void
*/
public function __destruct()
{
curl_close($this->ch);
}
/**
* Method sets a flag as to whether the object is in test mode
* @return void
*/
public function test()
{
$this->test = TRUE;
}
/**
* Check to see if this tweet has already been posted and add it to the DB
* if it has.
* @param string $message The tweet
* @return bool
*/
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;
}
/**
* A public method to post a tweet. If in test mode, this method will email
* the tweet instead of posting it.
* @param string $message The tweet to be posted.
* @return bool
*/
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) || ($this->res['http_code'] == 200))
{
return TRUE;
}
return FALSE;
}
}
?>
This isn’t perfect, but it’s better. Next we’ll examine refactoring the cURL components, including abstracting them out and improving the behavior of the constructor function.
Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP
Posted on 8/17/2009 at 12:00 am
Nice article. The world will be so different if every PHP beginner takes coding standards serious.
Ralph Schindler (@ralphschindler) wrote at 8/17/2009 12:15 pm:
One note, perhaps its just a small note:
Generally, when creating classes in PHP, and using PHP’s OO-goodness, I like (as to PEAR and ZF standards) to name members such as methods and properties with camelCasing. Thus “already_tweeted” would be “alreadyTweeted”.
On the other hand, I like to use under_scored naming for procedural paradigm members: functions and variables, when they are used in a predominantly functional/procedural/imperative environment.
Brandon Savage (@brandonsavage) wrote at 8/17/2009 12:30 pm:
I also tend to use the camel casing, and that’s a good point that I missed. To me, that’s less important than items that can cause big problems later on, but you are 100% right.
I also didn’t address making the variables easier to read and understand by just looking at them. I didn’t want to open that can of worms.
Thanks for the suggestion!
evll wrote at 8/17/2009 1:55 pm:
“But there’s something else about the properties that we need to examine. Because the properties are private, this class cannot be extended. This might be an intentional architectural choice; however, it is more likely that the author just didn’t consider the possibility of extension.”
Soooo wrong. protected is no more encapsulated than public. Suppose we have a public data member, and we eliminate it. How much code might be broken? All the client code that uses it, which is generally an unknowably large amount. Public data members are thus completely unencapsulated. But suppose we have a protected data member, and we eliminate it. How much code might be broken now? All the derived classes that use it, which is, again, typically an unknowably large amount of code. Once you’ve declared a data member public or protected and clients have started using it, it’s very hard to change anything about that data member.
noel darlow wrote at 8/17/2009 9:20 pm:
Personally I hate doc blocks. They don’t tell me anything useful. Take this for example:
/**
* Constructor.
* @param string $username The username of the Twitter user
* @param string $password The password of the Twitter user
* @param string $email Optional email address for testing
* @return void
*/
public function __construct($username, $password, $email = null)
{I can see it’s a constructor. I can guess what “$username” is because it’s well-named, and I can guess that it’s a string. Ditto all the other args. You could delete the lot without losing any useful information. I think *the* key programming skill is learning to express ideas clearly, simply, and concisely. That means constantly cutting and hacking away at cruft.
Short methods which do just one thing help. If this waas split into a _hasBeenPosted($tweet) and _saveTweet($tweet) (roost?) a comment isn’t needed.
/**
* Check to see if this tweet has already been posted and add it to the DB
* if it has.
* @param string $message The tweet
* @return bool
*/
private function already_tweeted($message) {Good naming virtually removes the need for comments – also testing. Code is best explained in well-factored unit tests which tell little stories about how classes are used and how they will behave. I think this is where the focus should be when you want to learn (or show) how code works. Comments may get out of step with the code but tests never lie.
Definitely agree that private is evil (open-closed principle).
And always use accessors. I’m a Scot and as we know Scotsmen all wear kilts hoots mon och aye the noo etc etc. For better or worse, it seems there is unending interest in what we wear underneath. Now, if you met me, I’m sure you’d ask politely using my public ear interface. You wouldn’t just hoik up my kilt and have a good old rummage around, leaving me exposed to the inclement Scottish summer.
I sort of feel the same way about direct property access.
Noel, the DocBlocks aren’t for when you’re reading the code. It’s for when you’re generating documentation. For example, I work with a large body of documentation, all of which is docblocked. I can instantly generate documentation that shows me the entire public API.
DocBlocks are not, in my opinion, for the person reviewing the code (beyond some basics). They’re for someone looking at the API.
And good point about the public properties versus accessors. That’s my take, too.
Tarique Sani (@tariquesani) wrote at 8/18/2009 2:00 am:
One tool that I have found really helpful in ensuring coding standards is PHP_CodeSniffer in the SVN/GIT pre-commit hook. You can’t commit if you don’t comply :D
I’ve seen the pre-commit hook standards check, and while I think it’s not a bad idea I don’t use it. Here’s why:
* If you have lots of code to commit you can potentially cause the SVN commit process to “hang.”
* You can use CI (continuous integration) to instead check the standards and email out notices to the staff if someone checks in a bad copy of the code.I do recommend a pre-commit hook that checks to make sure code lints (that is, it compiles) but beyond that, I think the work should be done post-commit. See http://blueparabola.com/blog/subversion-commit-hooks-php for more.
| « My Essential WordPress Plugins | Peer Review: Taking Code And Making It Better » |