Stop ACTA

Code formatting and readability

Posted in CakePHP on 10.07.2008.

While rewriting bits of NeutrinoCMS, I've noticed a slight error in judgment on my behalf. I was following what I consider to be bad code formatting, though some will surely disagree.

What I'm going to talk about here is the pseudo-principle I like to call "ending on a positive note". Basically, I believe that one should always try to write functions in a way that they end with a positive result. This is often not the case in CakePHP applications I've seen.

To explain further, let take a look at these two diagrams:

I know they are a bit abstract (it's late and I'm a bit tired) but bear with me.

When I first started learning CakePHP a while ago, I was looking at the simple blog tutorial and similar. In many of those tutorials, I've noticed a pattern in the code, a pattern like this:

function add()
{
    if (empty($this->data))
    {
        if ($something)
        {
            if ($this->_doSomethingElse())
                $this->_wellDone();
        }
    }
    else
    {
        // handle post here...

        if ($this->Cabbage->good())
        {
            if ($this->Cabbage->neighbourNotAtHome())
            {
                $this->Cabbage->stealAndEat();
            }
        }
    }
}

And without a thought, I've started implementing it myself. I was going through my code a few weeks ago and realized it's all wrong. My style of coding was terribly broken! What happened?

Let's see an example from the alpha release of NeutrinoCMS (I've removed some irrelevant lines for clarity):

function add($slug = null)
{
    $this->_setCategories();

    if (empty($this->data))
    {
        if (!empty($slug))
            $this->data['Article']['title'] = $slug;

        if (isset($this->passedArgs['category']))
        {
            /* snip */
        }

        if (empty($this->viewVars['categories']))
        {
            /* snip */
        }
    }
    else
    {
        $this->Article->data = $this->data;

        if ($this->Article->validates())
        {
            if ($this->Article->save($this->data))
            {
                $this->Session->setFlash('Article saved');
                /* snip */

                $this->_redirectTo(/* snip */);
            }
            else
            {
                $this->Session->setFlash('Article was not saved!');
            }
        }
        else
        {
            $this->Session->setFlash('Please correct the errors below');
        }
    }
}

So, if you look at this code carefully, you'll see that conditions are all set in the form of

if (success)
{
    if (success2)
    {
        if (doSomething())
        {
            // we're fine
        }
        else
        {
            // failed in doing something
        }
    }
    else
    {
        // possibly fail here
    }
}
else
{
    // or finally fail here
}

So basically, the last bit of code of your function is a failed operation. In the above example, it's the 'Please correct the errors below' message.

Now, take a look at this rewrite from the beta release of NeutrinoCMS:

function add($slug = null)
{
    $this->_setCategories();

    if (empty($this->data))
    {
        if (empty($this->viewVars['categories']))
        {
            /* snip */
            $this->redirect(/* snip */);
        }

        if (!empty($slug))
        {
            $this->data['Article']['title'] = $slug;
        }

        if (isset($this->passedArgs['category']))
        {
            /* snip */
        }

        return;
    }

    $this->Article->data = $this->data;

    if (!$this->Article->validates())
    {
        $this->Session->setFlash('Please correct the errors below');
        return;
    }

    if (!$this->Article->save($this->data))
    {
        $this->Session->setFlash('Article was not saved!');
        return;
    }

    $this->Session->setFlash('Article saved');
    /* snip */

    $newSlug = $this->Article->getSlug($this->Article->id);

    /* snip */

    $this->_redirectTo('view', $newSlug);
}

Did you spot the key difference?

All the crucial conditions are reversed! What does this mean? Is this code more readable? I'd say yes. It is easier for me to follow the following:

if (!sucess)
    return with error;

if (!sucess with something else)
    return with a different error;

// other code, finish process

end function with sucess;

Instead of:

if (success)
    if (other success)
        // other code
        end with success;
    else
        return with error;
else
    return with error;

One obvious advantage is the level of nesting, where "positive code" has a great advantage in terms of readability. One possible flaw could be the danger of code duplication if this style of writing is misused. But then again, the problem might be the author of the code and not the code itself... ;-)

I hope this will make you think about your coding style and the logic we (humans) use to interpret the code in front of us. The extra nesting used in the "negative" style is visually harder to follow and it's a bit illogical for a function to end with a success in the middle of it, and flash a failure at the end. I also find it very nice visually to see a function ending with (symbolical) "return 0" at the very end.

I know this style is not always applicable, but there are a lot of places where it is. I apologize if I'm a bit vague in this article, I hope you got the point, if not - or if you disagree, let me know...

Article comments — View · Add


Page 1 of 1

Martin Westin :: 12.10.2008 05:17:04
THis is funny enough exactly the conclusion I have come to. It is nice to see I am not just crazy... or at least that I am not alone in my madness :)

It was when I started having a few methods with a lot of ifs that this really hit home for me. I do have one criticism. This kind of "vertical" style works great for controller logic, but often not so great for validation-type code.

When you have a method that is validating you would probably want the result to reflect ALL problems with the data validated, not just the first error encountered. Forms that only give me one error at a time really irritate me.
Your name is missing!... ok. Now your email is formatted wrong... ok then. you need a zip-code... bloddy **"#&!!

But when it does not affect things like this it is much easier to read.
lecterror :: 27.10.2008 10:35:36
I'm glad you agree Martin, and as you pointed out well, sometimes this style would basically drive the end user mad. And since we always have to think about the end user, sometimes this is a big no-no. :-)

On the other hand, validation is a different kind of animal so maybe another article is due.. ;-)