Coding Style – A Fine Line Between Clever and Stupid Part 2

Continuing from my previous post, I wanted to write about something that I have found to be a bugbear – the idea that functions or methods should have a single exit point.

But first, another quick reminiscence of my days programming mainframes with PL/I. There were no internet connections to the machine, data came in on tapes. Someone who wanted to get their own nefarious data onto our machine would have had to intercept a driver, get the relevant tape, take it to a conveniently located mainframe resource, load the data off the tape, insert their own data into the records, put it back onto tape, drive it to our office with a driver with an appropriate security pass… and so on. And I didn’t even mention knowledge of record formats or file validity checks and so on!

Perhaps unwisely, the general principal we worked under was that data we processed was from trusted sources; and that meant we rarely had to get distracted by a massive overhead of cross-checking and validating every single record. Compare that to a user-interface or web environment today where every input has to be checked and protected from malicious users!

Now also consider that your mindset as a GUI programmer concerned with protecting the program from the malicious user is probably different from the mindset you need to process the information in whatever way your system requires is also different. Certainly, the latter will require some amount of caution and defensive programming, but if the UI programmer has done their job, one would hope the guts of the system will be protected from system-crashing data. Surely, there are at least two required mind-sets here? And if you happen to be working alone, you’ll have to switch between those mind-sets to do a good job.

All this leads to the main topic of this post; the rule that seems to be referenced on many occasions that you should have only one exit-point from a procedure or function. I simply don’t agree!

Here’s how I tend to write functions, imagining a function that returns a boolean success / failure variable, but that operates on global data in some form (apologies or lines that I’d like to be blank being commented; the blog editor I use doesn’t like retaining spaces, blank lines etc):

bool processSomething(params...)
{
    // Test parameter validity and fail if invalid
    if (param1 == blah && param2 > blah2)
        return false;
//
    if (param1 == blah && param2 <= blah3)
        return false;
//
    // From here on, have warm fuzzy feeling
    //  that the data is valid!
    // [Do the real processing here part 1]
    if (someErrorInProcessing)
        return false;
//
    while(someOtherCondition)
    {
        // [Do the real processing here part 2]
    }
//
    return true;
}

You might think me a little cheeky for putting a ‘return true’ as the last line of that function. What about if there is a failure in the real processing? Well, in the approach to coding shown above, any failure can / will be followed immediately with a ‘return false’. If we get to the end of all the processing, then everything should have worked ok.

Also, what about the ‘some error in processing’? Well, this is a contrived example, and perhaps not a great one 😉 But there may be some cases that error can pass the validity checks that can be done at pre-processing stage, but be invalid or un-processable for other reasons – imagine for a moment that instead of returning a boolean, this method returned an integer error code (one for each exit point) and it starts to look a little more reasonable.

In contrast, look at what we might need to do if we wanted a single point of exit – I need to add a little bit of imaginary code for the processing section here:

bool processSomething(params...)
{
    bool processingOk = true;
    // Test parameter validity
    if (param1 == blah && param2 > blah2)
        processingOk = false;
//
    if (processingOk &&
        param1 == blah &&
        param2 <= blah3)
        processingOk = false;
//
    if (processingOk)
    {
        // Parameters were ok
        // [Do the real processing here part 1]
        while (processingOk && someOtherCondition)
        {
            // [Do the real processing here part 2]
        }
    }
//
    return processingOk;
}

This is a very simplistic example, but hopefully it emphasises the issue that now, with a single exit point, we are having to think about error data or error conditions throughout the entire function… and I have not even considered multiple types of error condition or throwing exceptions; Is it acceptable to throw an exception at the start of the function, or should you set a boolean and throw it at the end? I was also a little bit cheeky on the second ‘parameter check’ as I added a test against the processingOk boolean. If our tests were simple range checks as shown this would probably not be considered necessary; but if the tests were time-consuming then it might be quite reasonable to do this.

There is another minor problem with this model: our return value variable has to be set true to start with. It’s not so bad when it’s named processingOk, but if you’ve ever used this type of code with a variable named ‘retValue’ or something similar, then the apparent implication could be that the return value will be true, when of course you can’t know that.

In contrast, the first example has no such variable, but the coding style dictates that ‘if you are running this line of code, all prior tests and checks have passed ok’.

Having written the body of this article, I happened to find this link that expresses a similar opinion, though in Java. If you read the comments and follow the link to the follow-on article you’ll see that the commenters are not impressed 😉 – but at least one thankfully suggests that throwing Exceptions at the start of a method is acceptable. A worthy point is also raised about possible needs to check post-conditions in a function; and perhaps in that case the approach suggested above is not the best.

I hope that this blog and the links above provide enough food-for-thought to suggest that single exit points may be overloading our brains with stuff we could have dealt with by simply bailing-out with a simple ‘return’… giving us the far more straight-forward task of dealing with ‘good data’ later in the function or method. The second article noted above also helpfully points out that this approach can be used in pretty-much any kind of block (loops, try-catch etc.) in certain circumstances.

One thought on “Coding Style – A Fine Line Between Clever and Stupid Part 2

  1. In my 20 years of coding I learned that there are “elegant” academic approaches and then there are practical ones. Having “exit as soon as you can” strategy is the latter. I didn’t know people still debate this.

    The way I see it, a good code strikes a balance between saying too much and being too abstract. When I’m looking at the code, I’m not reading a book — I’m trying to understand what comes out given certain inputs, preferably in one pass. Having early exits helps to eliminate all dead branches early in the process and spear me a headache. In case of search-like function, it goes as follows.

    1. If parameters are bad, who cares what the rest does? Out.
    2. Now, that hands are clean, let’s see what’s for dinner. What is a default value that will be returned in all cases that I don’t know or don’t care about?
    3. Do actual work. Return as soon as found, or set it as a return value possible post-processing and exit any loops. Forget about everything else.
    4. Return whatever is set to be returned, because at this point if it’s there, it must be correct and I don’t remember how it got there.

    The only thing that still bothers me is throwing exceptions vs. returning nulls and default values. But that’s a different problem.

Leave a Reply

Your email address will not be published. Required fields are marked *