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.
The topics discussed in this entry may be fairly advanced. Please feel free to ask questions, and discuss best practices.
If you’ve been following this series from the beginning, take a moment to look at the original code sample and compare it with where we are now. We’ve come a long way!
There is one last area that I want to address, and this has everything to do with object-oriented principles and code reusability. For those who are familiar with OO programming, they realize that the use of classes does not make something object oriented by nature. In this final part of the series, we’ll move one step closer to being object-oriented, by introducing the concepts of request and response objects.
At the moment, our object takes arguments like most functions do. This has some limitations. The first limitation is that the object must be aware: that is, it must have an understanding of the request it is being passed, and the response that it is getting from Twitter, as well as the response it will give back to our application. This means that in the event that something ever changes about the way that response is organized, we have to change this code, explicitly. I would like to avoid that.
(more…)
Monday, September 21st, 2009 @ 1:00 am |
Comment (6) |
Categories: Best Practices, System Architecture, PHP 5
Tags: refactoring, OOP, Peer Review, code review
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:
(more…)
Monday, September 14th, 2009 @ 1:00 am |
Comment (4) |
Categories: Usability, Best Practices, System Architecture
Tags: architecture, PHPUnit, testing, unit testing
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.
(more…)
Tuesday, September 8th, 2009 @ 1:00 am |
Comment (10) |
Categories: Best Practices, System Architecture, Debugging
Tags: business logic, design, objects php 5, PHP 5
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.
Editor’s Note: The response of the community to this series has been great, and I’ve been given a large number of suggestions. I’ve incorporated some of those suggestions into the code and into this article. Thanks to Jeff Carouth, Greg Beaver and Daniel O’Connor for their help and suggestions.
This entry will focus on our use of the database, and specifically on the already_tweeted() method. This method has a number of problems, and while we’re focusing on the implementation of the database, it’s important to note that we will also need to address some of the logic (which will be the next part of the series).
In our last entry, we focused on abstracting the HTTP request out to a seperate class. Lots of people wrote comments with suggestions of HTTP handlers, including pecl_http, the PEAR HTTP class, HTTP_Request2 and the PEAR Log class for logging. These are all great suggestions, and all will help abstract out the class without causing us to have to write our own implementation of common problems (the Not Invented Here (N-I-H) syndrome).
In focusing on the already_tweeted method, one thing becomes immediately apparent: it is private. This suggestion, provided by Greg Beaver relates to our first discussion of coding standards and we will change the class to a protected class for extendability later on.
(more…)
Monday, August 31st, 2009 @ 6:30 am |
Comment (2) |
Categories: Debugging, Best Practices, System Architecture
Tags: OOP, PHP 5, Propel
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.
There are a number of fundamental concepts in object-oriented design that we should take notice of. One of these concepts is abstraction. This is what we will focus on today and in the next entry.
This article will focus on the constructor method. There are a couple of problems, namely that the constructor itself does a lot of actual work. Also, we have the cURL setup done in the constructor. This object is a Twitter object, not a cURL object; this means that we should decouple the cURL functionality and abstract it into a separate object of its own. This not only will make our object more true to it’s functionality as a Twitter object, but will allow for greater reuse (since we’ll be able to use the cURL object for more).
There’s some debate about whether or not you ought to write things like wrappers for native PHP functionality. One thing that is missed in the “don’t write a wrapper for cURL!” argument is that a good wrapper for HTTP requests shouldn’t be limited to cURL. In fact, it should make use of fopen(), file_get_contents(), etc. in the case that cURL doesn’t exist. A good object would test for this, and change its behavior based on preset rules. (strategy pattern, anyone?)
(more…)
Wednesday, August 26th, 2009 @ 1:00 am |
Comment (11) |
Categories: Best Practices, System Architecture
Tags: PHP, objects php 5, refactoring
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.
(more…)
Monday, August 17th, 2009 @ 12:00 am |
Comment (8) |
Categories: Best Practices, System Architecture, PHP 5
Tags: code review, constructor, refactoring
| « Older Entries |