Bryan Zarzuela wrote at 9/14/2009 3:19 am:The constructor parameters in line 35 doesn’t match the phpdoc block
| « Custom Apps: Some Strategies For Easy Configuration Files | Train Your Employees » |
This entry is part of an ongoing series involving the review of a code sample and it’s refactoring. For the original code sample, see here.
Now that we’ve worked out the abstraction issues and the logic questions, we should take a moment to focus our attention on a few of the issues relating to the architecture and testability of the class we’ve worked out.
A couple of big architecture issues raise their heads early on. The first one is this set of code:
<?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;
}
[/sourcecode]
This raises a number of architectural issues that we should address. My recommendation is that we remove this code altogether. The first line, about error reporting, really should be set at the application level, rather than the script level. As for the redirect, this is intended to prevent the script from being called directly. However, it's a good idea to place classes like this outside the document root anyway, meaning that they would never be called directly. When including this file, there's the potential that you'll not have included enough files and you'll inadvertently redirect your user. So let's drop these lines altogether. This will improve reusability and improve the architecture.
There's another line of code that is particularly troubling:
[sourcecode language="php"]
$this->http->setHost($this->baseHost . "statuses/update.json?status=".urlencode(stripslashes( urldecode($message))));
It may not look obvious at first, but this code relies on the assumption that the magic_quotes_gpc directive is set to on.
For those who don’t know, magic_quotes_gpc automatically adds slashes to all GET, POST and COOKIE variables that come into an application. This deprecated feature represents a poor programming practice, and its use is discouraged. Though it remains on by default in PHP installations, it should be turned off altogether if at all possible. It’s slow, and potentially dangerous. Additionally, it will be removed in future versions, meaning that code relying upon magic_quotes_gpc will break in the future.
Now, oftentimes programmers don’t have direct access to the magic_quotes_gpc directive in php.ini, but it can be set on the htaccess level, if your hosting provider allows you to access PHP variables in this way.
I’m going to make the assumption that we’ve disabled magic_quotes_gpc, as is recommended by the PHP manual.
Another problem that we need to address is the fact that this object gets thrown away after it’s been used. We have the property $done, which gets set when a tweet is successful. Unless there’s a good reason for it, objects should never be designed this way. We’ll remove this code in the final draft (see below).
Moving on to testing, again we have to ask ourselves about abstraction. Where testing is concerned, it would be very easy to abstract the testing out of this object. Making use of things like the HTTP_Request2_Adapter_Mock class will help us to “test” the Twitter interface without actually posting a tweet. Unit testing software like PHPUnit will give us reports on code coverage.
If we opt to leave the test code in the object itself, it should be abstracted into its own method. But I don’t recommend this course at all, because testing should be conducted at the application level. Moving testing out of the class also allows us to remove references to the $done property, and remove the $you property as well as the test() method, the $test property, and a large bit of code from the tweet() method. All of this code will then not be loaded on each request of our class. Not a bad improvement.
Something else that we’ll do in order to improve testability is we will inject (rather than create) the HTTPRequest object into the class. This allows us to both inject an object that is a mock object, and it also allows us greater control over the environment when we do our testing.
It was pointed out by the blog PHP In Action that one of the things that we should have done first is write unit tests. I think that this is good advice for most applications. Obviously, the small size of this class make unit testing less critical, but it is a good practice to write unit tests first, and then refactor.
A good example of this is the work being done by web2project with regards to bug fixes. According to Keith Casey, one of the leads on the project, their rule now is that any time a bug is fixed, a unit test identifying the bug must be submitted along with the bug fix. This has allowed them to significantly improve their testing system, and it’s a great idea for existing projects that don’t have unit tests but want to implement them.
Testing is not to be underestimated. Testing allows you to refactor methods and the internal workings of a class, and still know that the results are right. It allows you to work on a very small part of an application without necessarily understanding the whole application, because you can run a unit test suite and see if your code works properly. And writing tests first, even on bug fixes, forces you to think about how you expect your code to work, and face the assumptions you’ve made about that process, as well.
We’ve made a good number of changes to the code, and we’ve made a vast number of improvements. Let’s take a look at what we’ve got:
<?php
/**
* Twitter Class
* @author Anonymous
*/
class Twitter
{
/**
* @var HTTPRequest HTTPRequest Object.
*/
protected $http;
/**
* @var string The username for the Twitter account.
*/
protected $user;
/**
* @var string The password for the Twitter account.
*/
protected $pass;
/**
* @var string The base host for tweeting
*/
$baseHost = 'https://www.twitter.com/';
/**
* Constructor.
* @param HTTPRequest $request The HTTP request object.
* @param string $username The username of the Twitter user
* @param string $password The password of the Twitter user
* @return void
*/
public function __construct(HTTPRequest $request, $username, $password)
{
$this->user = $username;
$this->pass = $password;
$this->http = $request;
}
/**
* A destructor.
* @return void
*/
public function __destruct()
{
unset($this->http);
}
/**
* Check to see if this tweet has already been posted.
* @param string $message The tweet
* @return bool
*/
protected function checkTweet($message)
{
$hash = md5( date('Y-m-d') . $message );
$c = new Criteria();
$c->add(TwitterLogPeer::THASH, $hash);
$count = TwitterLogPeer::doCount($c);
if($count > 0)
{
return TRUE;
}
return FALSE;
}
/**
* Log the tweet if it has already been tweeted.
* @param string $message The tweet
* @return bool
*/
protected function logTweet($message)
{
$hash = md5( date('Y-m-d') . $message );
try {
$logTweet = new TwitterLog();
$logTweet->setDate(time()); // This will be turned into a DateTime object.
$logTweet->setHash($hash);
$logTweet->setTweet($message);
$logTweet->save();
} catch (Exception $e) {
throw new Exception('There was an error in saving the item to the database.');
}
return TRUE;
}
/**
* 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))
{
return TRUE;
}
// STATUS UPDATE ON TWITTER
$this->http->setHost($this->baseHost . "statuses/update.json?status=" . $message);
$this->http->setCredentials($this->user, $this->pass);
$this->http->setOption(HTTPRequest::RETURN_TRANSFER, true);
$this->http->setOption(HTTPRequest::POST, true);
$response = $this->http->execute();
$array = json_decode($response->getRaw(), true);
if(isset($array['id']) AND $array['id'] > 0)
{
return $this->logTweet($message); // returns true.
}
return FALSE;
}
}
?>
Next, we’ll delve into the advanced topic of request and response objects, and discuss how this can improve our understanding of the results the Twitter object delivers. This will be our final entry in the series, and will wrap everything together.
Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP
Posted on 9/14/2009 at 1:00 am
The constructor parameters in line 35 doesn’t match the phpdoc block
Brandon Savage (@brandonsavage) wrote at 9/14/2009 9:42 am:
You’re right! +1. I updated the docblock and it’s now correct.
This is a great point, and it shows the need for something like CodeSniffer to help ferret out those issues. CodeSniffer would have picked that up right away. I recommend a post-commit hook that runs it, and emails errors to the team for correction (a PHP lint hook should run on the pre-commit side).
CodeSniffer comes with a bunch of attachments, but you can also write your own standards. It’s part of PEAR, so you can get it that way. http://pear.php.net/package/PHP_CodeSniffer/
Jeff Carouth (@jcarouth) wrote at 9/14/2009 10:35 am:
Since you removed the testing bit from the tweet() method you should also remove the documentation from the corresponding docblock. ;)
I could not agree more on having a test suite and also building and improving the test suite with each bugfix. On my team it is required to first create a test case to reproduce the bug. I usually get dirty looks from developers when I say to do this mainly because the process can be tedious. However, it would be impossible to count the number of times that a “simple bug fix” turned out to uncover some more complex issues when taking the time to thoroughly and consistently reproduce the bug with a test case.
If your project doesn’t have any unit tests yet, that’s ok. When you fix a bug is a great time to start creating one. ;)
Thanks Jeff. I did in the final post (coming on Monday).
| « Custom Apps: Some Strategies For Easy Configuration Files | Train Your Employees » |