PHP in Action

How code comments deteriorate

dagfinn — Sun, 04 Jan 2009 03:24:12 -0800

There was a lot of disagreement on the value of code comments after my earlier post Comments considered harmful. Perhaps the most important objection that was raised was the idea that it's OK to improve the code, but it's even better to keep the comments in addition to the improved code.

As one of the critics expressed it:

No, comments are there to comment. Period.
That has nothing to do with how good is your code.
You can write perfectly clean code and add good comments to it. Nothing wrong with that.

This is one of those ideas that seem obviously true in theory, but fail to work in practice. The reason is that comments get out of sync with the code. The comments rot, or rather, their meaning does. They become more and more misleading as the code gets changed and the comments are not adequately updated.

Let me give you an example. But first, I want to make it clear that I don't think code comments are always a bad thing. Sometimes, they are necessary. But much less often than people think.

API documentation is often indispensible, but that's really a different matter. When API documentation is generated from comments in front of each method, the primary purpose is to explain how the code can be used, not how it works. In fact, you might say they are code comments only in a syntactical sense.

On to the example. One of the comments to my blog post presented a code snippet that illustrates my point well. It's supposed to be a example of a useful inline comment in code.

// first handle the case where no records were found
(if $records == 0) {
    return false;
}

Is this really an case of good commenting? I don't think so. If it's hard to understand that $records == 0 means that no records were found, we can change the code to make it easier. The simplest way to do it is to rename the variable to something like $numberOfRecordsFound or $numRecordsFound. Or extract a method. But typically, it's possible and preferable to avoid this kind of check altogether.

Anyway, the comment is unnecessary. But as I only realized a while later, it's also misleading. Does the code "handle the case where no records were found"? No, it leaves it to the calling function to handle the case. The one line that returns false is not handling anything, it's just passing a message.

So the comment is already misleading, never mind what will happen when someone changes the code and neglects to update the comment. For example, let's say the next programmer to work on this code is in hurry to fix a bug. Now maybe the code will look like this:

// first handle the case where no records were found
(if $records == 0 && $state == ACTIVE) {
    return false;
}

The programmer wonders briefly whether the comment is still fully appropriate, is unsure, and decides to do nothing about it (or postpones the decision and forgets about it). Now, perhaps (we can't quite tell without the full context), the comment is even more misleading.

In the next round, yet another programmer comes along, makes some more code changes and wonders: "Should I update the comment? It looks all wrong to me, but it was probably written by someone with deeper insignt into the code. Better leave it alone."

The code would have been better off without the comment, but no one wants to delete it, especially since they have been told that comments are so important. It's a downward spiral: the code changes make the comments misleading less chance that they will

Comments that lie are worse than no comments, and in practice they tend to big liars.

How harmful is "harmful"?

dagfinn — Fri, 26 Dec 2008 05:14:15 -0800

Conflict is not a bad thing. That is, if it leads eventually towards clarity and understanding rather than confusion and misunderstanding. And it's almost always better to have a conflict openly than to sweep it under the rug. Fear of conflict is the second of the five dysfunctions of a team.

And even though anger tends to cloud people's judgment, it need not be terribly destructive. If the angry person can manage to cool down and eventually keep an open mind in spite of it, it's not so bad. The problem is that people often get stuck trying to prove that they're right, placing blame and defending whatever position they started with at all costs.

One factor that exacerbates this is when different discussions get mixed up. When discussions get heated, you will often you get a discussion about the discussion mixed up with the original discussion. A debate about a technical matter may be conducted in parallel with an altercation that addresses the relationship between the participants. This is confusing in itself. Keeping these discussion apart will help maintain clarity.

In this vein, let me address the criticism I got in my latest blog post ("Comments considered harmful") regarding the appropriateness of the "considered harmful" idiom. This is not a technical issue, so let me give it a separate treatment.

A response I got was:

If ever there was a posting to which the "Considered Harmful" Essays Considered Harmful posting applied, it's this one.

To quote my own reply:

I have read that before. I fully understand the argument and agree with it in general terms. In particular, I agree with the idea that 'the writing of a "considered harmful" essay often serves to inflame whatever debate is in progress..'

In this case though, there seems to be no debate in progress in the first place. I am trying to make my point strongly enough that people will at least notice it.

If the rest of the blog post had been over the top, then I would probably have been shooting myself in the foot. I don't think it is, though.

I find the idea of "considered harmful considered harmful" valid, but over-generalized. Sometimes, a little conflict is needed to wake people up. Sometimes, you need to have two opposed camps for a while to make the difference of opinion clear before you can come to agreement.

When it comes down to it, it's an empirical question. I choose to measure the success, or lack of it, of my provocative title by the actual responses I get. Few of the commentators seem to have been derailed by the title. Some had specific technical objections, and that's fine even when I don't agree with the objections. Some asked me how specifically comments might be harmful. That also is totally relevant and, in fact, helpful.

Let me try to be even more precise. My aim in using this kind of title is not primarily to provoke, but to (over)simplify the message to make it easy to get some idea of the content by reading the title. If I had said "comments are not always useful", it would be so vague that anyone could embrace it. I would rather make it clear that I'm pushing a point of view that many are going to disagree with. Maybe I'll choose differently the next time, but I'm not promising anything.

Comments considered harmful

dagfinn — Tue, 23 Dec 2008 11:11:54 -0800

There is too much old advice in PHP. A recent case comes from the PHP Advent calendar. Eli White is a strong believer in commenting code, including inline comments inside functions.

Unfortunately, he's at least 10 years too late. This used to be good advice, but not any more.

Up to a point, he's right. Making code as easy to understand as possible is essential and can save a huge amount of time later. And adding comments to unreadable code is better than leaving it the way it is.

There is a better way, though. Refactor your code so it's easy to understand even without comments. I'm just reading Robert C. Martin's recent bookClean Code. He is very clear on this point.

Clear and expressive code with few comments is far superior to cluttered and complex code with lots of comments. Rather than spend your time writing the comments that explain the mess you made, spend it cleaning that mess.

The principle is simple: if you have an inline comment in a method (or function), take the chunk of code that the comment refers to and extract it into a separate method. Give the method an intention-revealing name. Typically, you won't need the comment afterwards.

I'm sure we need an example. Here's a small excerpt from a method taken from Zend Framework.

    protected function _doUpdate()
    {
    ...
        /**
         * Execute cascading updates against dependent tables.
         * Do this only if primary key value(s) were changed.
         */
        if (count($pkDiffData) > 0) {
            $depTables = $this->_getTable()->getDependentTables();
            if (!empty($depTables)) {
                $db = $this->_getTable()->getAdapter();
                $pkNew = $this->_getPrimaryKey(true);
                $pkOld = $this->_getPrimaryKey(false);
                foreach ($depTables as $tableClass) {
                    try {
                        @Zend_Loader::loadClass($tableClass);
                    } catch (Zend_Exception $e) {
                        require_once 'Zend/Db/Table/Row/Exception.php';
                        throw new Zend_Db_Table_Row_Exception($e->getMessage());
                    }
                    $t = new $tableClass(array('db' => $db));
                    $t->_cascadeUpdate($this->getTableClass(), $pkOld, $pkNew);
                }
            }
        }

By changing some temporary varibles into instance variables and extracting a couple of methods, we can get code like this:

      if ($this->primaryKeyValuesHaveChanged() {
          $this->executeCascadingUpdatesOnDependentTables();
      }

Or even, by extracting a couple of classes, making objects out of two of the concepts involved, we might end up with something like this.

      if ($primaryKeys->valuesHaveChanged() {
          $dependentTables->executeCascadingUpdates();
      }

This uses most of the words from the original comment, but it's superior for several reasons. First and foremost, the section of code as a whole is clearer. Second, comments have a tendency to get out of sync with the code. By expressing the message in the code itself instead, we can avoid that. Third, the extraction itself has the positive side-effect of making it possible to test the extracted method in isolation.

Improving the code itself is harder than writing comments, but it's worth it. .

The one-line web framework

dagfinn — Sun, 14 Dec 2008 05:53:49 -0800

The core of your average web framework is a Front Controller. Front Controllers are commonly considered complex and esoteric. That's a myth. I sometimes brag that I can construct a Front Controller in 15 minutes. Actually, it's doesn't take quite that long. In PHP, a Front Controller can be simplified to just one line of code:

call_user_func($_REQUEST['action']);

Warning: Do not use this in a real application.

There is one and only one good reason why you shouldn't: it's totally insecure. Except for that, it's simple but perfectly viable. Your actions will be just plain functions, which is pretty simplistic and will be hard to handle if you build many of them. But it's a huge improvement over the average PHP script with no systematic handling of the PHP request.

If you want to do something more like the frameworks such as Zend Framework, you want your actions as methods grouped into classes (called Controllers in ZF).

Now you need two lines. The following script demonstrates this:

// Pretend we have an HTTP request
$_REQUEST = array('controller' => 'Contacts', 'action' => 'edit');
 
// Front controller
$controller = new $_REQUEST['controller'];
call_user_func(array($controller,$_REQUEST['action']));
 
// The action controller class
class Contacts {
    function edit() { 
        echo "ok\n";
    }
}

You can start with something like it (assuming that security has been taken care of) and add features as you need them.

In case you're wondering how to what exactly the security issues are, the most obvious problem is the fact that any user can run any method in any class that is available to this script. The way to avoid that append or prepend some fixed string to the class and the method name. For instance, in Zend Framework, the action name edit will activate the method editAction().

Beyond that, the security problems are the usual ones. You need to filter input, And in general you should beware of $_REQUEST, since it can also contain cookie data.

How to test everything

dagfinn — Thu, 27 Nov 2008 14:19:04 -0800

They say there's no free lunch, but at least there's free breakfast. Last week I attended a "breakfast seminar" with Robert C. Martin (Uncle Bob). There really was free food.

Anyway, Uncle Bob held an extremely entertaining and useful introduction to the FitNesse testing tool. He got me hooked on it, but I'm even more fascinated by something else. I asked him, "what is the nature of the test API [you've been talking about]?" He answered that question, and also another, more general one: What is a good strategy for complete automated integration and acceptance testing of an application? That was the question I really wanted to ask, only I wasn't quite aware that I wanted to ask it. And, impressively, he answered it anyway.

As I understood it, the strategy he outlined was as follows. The diagram is probably only a mild perversion of the one he drew on the blackboard.

Test all features through a test API just underneath the user inteface. These are the main acceptance tests; the ones that are used to decide whether a feature is done or not done yet.
Test the user interface in isolation, running calls to a fake system underneath.
Run just a few tests all the way through the application to test the plumbing between the user interface and the rest of the system.

The reason why we don't want all our acceptance tests to go through the user interface is that the user interface has a tendency to change often, and changes in the user interface tend to breaks lots of tests.

This resonates reasonably well with my experience, although I think testing "everything" through the user interface can work sometimes, if the user interface is a very plain one.

In addition, we want unit tests for each small piece of the application. The tests outlined above are just catch the few bugs that are not detected by the unit tests.

Zend_Test

dagfinn — Tue, 18 Nov 2008 14:45:54 -0800

I admit I don't follow Zend Framework very closely, since I haven't been using it for any serious work. But I did write a piece about testing a Zend Framework action controller with View Helpers.

This might need updating, since the testing capabilities of the Zend Framework have grown substantially since then. In particular, there is now a component called Zend_Test. I haven't had time to study it closely yet, but I hope to do so soon.

More beautiful code

dagfinn — Fri, 07 Nov 2008 20:34:42 -0800

I got some interesting comments to my previous post on "beautiful code". Some were pretty strong disagreements.

So am I wrong? Did I get carried away? Did my critical faculty go on vacation somewhere nice and sunny? I admit that sometimes I deliberately look at the positive and ignore the negative. (And sometimes I do the opposite; It's a good exercise if you're careful.)

I wasn't drunk, anyway. But let me take a closer look at the particular line of code I was praising:

$this->assertThat($form->hasSelect(withName('statusConfirm'))->hasValues(),
    array('Yes','No'));

My main point is that it's close to plain English. Not everyone agrees that that's a good thing, but I argue that we're built (genetically wired, in fact) to understand natural languages, not program code. Therefore code should be easier to understand when it approximates natural language and expression. And we're trying to create or approximate a Domain Specific Language (DSL), which should express exactly what's required for the domain and not the demands of the technical implementation.

So for this experiment, let's translate this one into a (plain English sentence:

Assert that Form (this particular one) has a select menu with the name "statusConfirm" and values "yes" and "no"

Translating back into code, it might look more like this:

$this->assertThat($form)->hasSelect()->withName('statusConfirm')->andValues('yes','no');

To me, this is even more natural than the other one. I think we've gotten rid of some syntax that has to do with implementation details rather than making the API simple to use.

It also seems clear to me how this could be implemented. All of the method calls could be to an assertion object that would take all these various inputs and always return itself at the end of the method so you can chain the calls in what's known as a fluent interface.

Beautiful code

dagfinn — Mon, 03 Nov 2008 04:36:56 -0800

Max Horwath has published his slides on Making Selenium Test Writing easier using a DSL onlinefrom IPC 2008. Let me quote the whole short description:

Implementing automated tests by using Seleniums API methods has several drawbacks. Selenium is great for what it does, providing a generic framework for testing a generic application. Using the Testing_SeleniumDSL framework, I will show you how to create your own Domain Specific Language (DSL), which would allow you to write tests in the language of your business rather than in Seleniums language.

I'm quite impressed by the examples he presents, such as:

$this->assertThat($form->hasSelect(withName('statusConfirm'))->hasValues(),
    array('Yes','No'));

This is truly expressive, readable code. I've refactored web test code in this direction many times, but I admit I never got quite this far.

The DSL is planned as an open source release. It would be interesting to try something similar for the SimpleTest web tester, which is my favorite for testing web interfaces without too much JavaScript. (Based on the paparrazzi principle.)

Refactoring is design

dagfinn — Sat, 11 Oct 2008 14:41:24 -0700

Refactoring is by definition a design actitivity, since the definition of refactoring is "improving the design of existing code". But is this generally and fully recognized? After attending my friendly local agile conference (Smidig2008—sorry, it's in Norwegian), I'm getting more of a feel for how different people think about it. And I'm wondering whether the use of metaphors such as "cleaning" makes refactoring seem too much like unskilled labor. After all, physical cleaning jobs are seen that way.

The analogy between cleaning and refactoring is useful for making the non-developers understand that refactoring is absolutely necessary. But beyond this pragmatic similarity, are the two really similar in deep and meaningful ways? I don't think so. Refactoring is not unskilled labor. It's a task that both requires and builds design skill and experience. While anyone can see that a floor is dirty, identifying code smells is non-obvious, tricky and demanding. This is true even of the simplest code smell, duplicated code. Although spotting code duplication is sometimes easy, at other times, the duplication is too subtle to be easily identifable. When you clean a floor, the goal is well-defined and easy to visualize. When refactoring, you may know what you're aiming for at each small step, but just a few moves further ahead you may end up with a structure you hadn't imagined.

Get links with XPath

dagfinn — Mon, 06 Oct 2008 12:53:11 -0700

There's a tutorial that appeared recently called Get Links With DOM. Planet PHP lists the author as Kevin Waterson, although his name is not mentioned on the page itself. Anyway, he claims:

Perhaps the biggest mistake people make when trying to get URLs or link text from a web page is trying to do it using regular expressions. The job can be done with regular expressions, however, there is a high overhead in having preg loop over the entire document many times. The correct way, and the faster, and infinitely cooler ways is to use DOM.

Yes, of course it's cooler. But I'm a little bit surprised at the claim that it's the "correct" (only) way, since there's at least one more that I find even cooler: XPath. Admittedly, it's slower, yet it's a more powerful language.

In his example, we just need to add a line to create an XPath object after we've created the DOM object:

$xpath = new DOMXpath($dom);

Then, instead of the DOM call:

/*** get the links from the HTML ***/
$links = $dom->getElementsByTagName('a');

we can use an XPath query:

/*** get the links from the HTML ***/
$links = $xpath->query('//a');

That's all. So why is that cooler? Because you can do more powerful searches easily. The DOM just happens to have a simple call to find all elements with a certain tag name, so there's not much difference in this case. But more complex stuff is something else. For instance, we can get just the URLs with a single expression:

$links = $xpath->query('//a/@href');

Or we can get just the URLs of just the links whose CSS class is "bookmark":

$links = $xpath->query("//a[@class='bookmark']/@href");

I've been using this for ages when testing web pages. Then there's the not quite official SimpleTest DOM tester, which uses CSS selectors to specify paths. But I won't go into that right now.