Jake Smith (@jakefolio) wrote at 9/8/2009 7:46 am:Another great article Brandon. Quick question.
Is there any reason why you chose to do an isset along with checking if the id was greater than 0? (line 173)
| « Train Your Employees | Peer Review: Looking At Abstraction – Redux » |
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.
So far, we’ve done quite a bit of work on our Twitter class, making it better. There’s still work to be done, though, especially improving the logic.
The Twitter class we have now has a number of logical flaws in it that we need to address. Additionally, there are some logical flaws that we started with that I want to highlight, even though we’ve already fixed them. Let’s get started with those.
When we started, we were setting the host (http://www.twitter.com/) in the constructor, and then concatenating it in the tweet method. But we were setting it as an object-level property. Let’s look at that code:
$this->host .= "statuses/update.xml?status=".urlencode( stripslashes( urldecode($message )));
This is a bad practice for one reason: it makes the object unusable a second time! If we ever use the object again, we’ll end up concatenating the message onto the end of the last message and getting a 404 error. Not good.
We’ve fixed that by setting the host inside of the HTTP object. Good.
I also have to ask: is an HTTP status code of 0 ever acceptable as a result? According to the author of this class, Twitter sometimes returned a status code of 0 but was successful in sending the tweet. However, Twitter ALSO returns a JSON or XML response, which is much more effective for determining the success (or failure) of a particular tweet. I’m going to assume that getting the response gives us pure JSON, and using the Twitter API documentation to make some assumptions. Let’s take a look at the bottom of the tweet() method:
// STATUS UPDATE ON TWITTER
$this->http->setHost($this->baseHost . "statuses/update.json?status=".urlencode(stripslashes(urldecode($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 TRUE;
}
return FALSE;
That’s much better. Now, we’re evaluating whether or not the tweet has an ID that is greater than zero, meaning that if anything else is returned, we know the tweet was unsuccessful.
We still have some logic issues to sort out, though. People who use Twitter know that occasionally it fails. If it fails while we are attempting to tweet, we could run into some problems because the already_tweeted() method inserts our tweet into the database BEFORE the tweet is actually sent. This is a huge logical problem.
Thankfully it’s an easy one to solve, by breaking the already_tweeted function into two functions: checkTweet(), and logTweet().
/**
* 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;
}
These logical fixes will improve the overall logic of the class. There are still a couple of considerations that the developer might want to take into account. The biggest consideration is that the process of verifying that we’re not sending the same tweet will work well, except over midnight server time. This is a small consideration, however, and you might consider whether fixing this presents a significant value.
Also worth considering is whether or not the object should even verify if we’re repeating ourselves; Twitter certainly can support this, and since we’re verifying that the tweet went through we can have error handling that notifies the end user. This, again, is a consideration for individual developers to decide. I would not verify this in the class, but others might, and I don’t want to remove it for that reason.
In the next part of the series, we’ll take a look at testing the class, and the class architecture.
We’ve made a large number of changes to the source code, so here is a complete set of the source code as it stands now:
<?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 HTTPRequest HTTPRequest Object.
*/
protected $http;
/**
* @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 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->baseHost = "http://twitter.com/";
$this->done = FALSE; // DEFAULT - NOT ALREADY TWEETED
$this->test = FALSE; // DEFAULT - THIS IS LIVE, NOT A TEST
$this->http = new HTTPRequest();
}
/**
* A destructor.
* @return void
*/
public function __destruct()
{
unset($this->http);
}
/**
* 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.
* @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))
{
$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->http->setHost($this->baseHost . "statuses/update.json?status=".urlencode(stripslashes(urldecode($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;
}
}
?>
Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP
Posted on 9/8/2009 at 1:00 am
Another great article Brandon. Quick question.
Is there any reason why you chose to do an isset along with checking if the id was greater than 0? (line 173)
Brandon Savage (@brandonsavage) wrote at 9/8/2009 7:54 am:
Jake, my understanding was that attempting to use a key that was undefined would emit a notice. However, testing on that seems to indicate that I am mistaken.
The isset() appears to be unnecessary. However, I might still keep it for two reasons: first, it explicitly checks to make sure that the ID value is set, and even though that condition would be satisfied by the second check, it’s obvious as soon as you look at it. Second, because undefined variables will emit errors (for example, if you test for the presence of $value but don’t define $value you get a notice), it’s a good practice to use isset() to test for value initialization before testing for the actual value.
Brandon, that is what I thought, but I wanted to make sure I wasn’t missing something. You’re completely right that it is good practice and should be implemented.
Once again, great series.
Bruce Weirdan wrote at 9/8/2009 4:55 pm:
>This is a bad practice for one reason: it makes the object unusable a second time!
Your object is unusable a second time anyway, since you’re not resetting $this->done.
You’re right, and you jumped a bit ahead on me. That fact is discussed in the architectural entry, which is next Monday.
Giorgio Sironi (@giorgiosironi) wrote at 9/9/2009 6:36 am:
I agree on the isset, everyone should develop in E_ALL | E_STRICT mode. What worries me is the $this->test variable where a mock object could be used.
I agree. We’ll be removing that next, when we write some unit tests.
Wil Moore III (@wilmoore) wrote at 9/15/2009 1:30 am:
Brandon,
Nice article. BTW, instead of this:
if(isset($array[‘id’]) AND $array[‘id’] > 0)
the following is more concise:
if (! empty($array[‘id’]))
!empty works just like isset in that it will check that the variable/array index is set but also has the added value of ensuring that the value is not an “empty” value like false, 0, or NULL.
One precaution though…if you are accepting 0 as a valid value, you don’t want to use empty because a value of 0 will cause empty to return false.
BTW, given the fact that you have set “error_reporting(E_ALL)” you most certainly will be getting a NOTICE if you omit the empty or isset call. If you didn’t see the notice, it could be that you have display_errors = false.
-Wil Moore III
Good point. And you’re 100% right. :-)
Albert Lombarte (@alombarte) wrote at 9/30/2009 3:48 am:
Hello Brandon,
I don’t know how I arrived here, nor I am using any of this techniques (I use multicurl for massive querying), but just a little bit of advise regarding the json_encode.
There is a bug with large integers and the PHP json_encode/decode functions in 32-bit machines (mere mortals usually use that). If you do not have a 64-bit machine the integers decoded will be corrupted, and you will have wrong twitter IDs.
You should either use a 64-bit machine or surround the numerical values with “” before decoding the response (treat them as strings).
My 5 cents
| « Train Your Employees | Peer Review: Looking At Abstraction – Redux » |