14

I don't know if there are any accepted names for these patterns (or anti-patterns), but I like to call them what I call them here. Actually, that would be Question 1: What are accepted names for these patterns, if any?

Suppose there is a method that accepts a bunch of parameters, and you need to check for invalid input before executing actual method code:

public static void myMethod (String param1, String param2, String param3)

Hurdle Style (guard clause, early return)

I call it so because it's like hurdles a track runner has to jump over to get to the finish line. You can also think of them as conditional barriers.

{
    if (param1 == null || param1.equals("")) {
        // some logging if necessary
        return; // or throw some Exception or change to a default value
    }

if (param2 == null || param2.equals("")) {
    // I'll leave the comments out
    return;
}

if (param3 == null || param3.equals("")) {
    return;
}

// actual method code goes here.



}

When the checks are for a certain small section in a larger method (and the section cannot be moved to a smaller private method), labelled blocks with break statements can be used:

{
    // method code before block

myLabel:
{
    if (param1 ... // I'll leave out the rest for brevity
        break myLabel;
    if (param2 ...
        break myLabel;
    ...

    // code working on valid input goes here

} // 'break myLabel' will exit here

// method code after block



}

Fence Style (single exit)

This surrounds the code with a fence that has a conditional gate that must be opened before the code can be accessed. Nested fences would mean more gates to reach the code (like a Russian doll).

{
    if (param1 != null && !param1.equals("")) {
        if (param2 != null && !param2.equals("")) {
            if (param3 != null && !param3.equals("")) {

            // actual method code goes here.

        } else {
            // some logging here
        }
    } else {
        // some logging here
    }
} else {
    // some logging here
}



}

It could be re-written as follows too. The logging statements are right beside the checks, rather than being after the actual method code.

{
    if (param1 == null || param1.equals("")) {
        // some logging here

} else if (param2 == null || param2.equals("")) {
    // some logging here

} else if (param3 == null || param3.equals("")) {
    // some logging here

} else {

    // actual method code goes here.

}



}

Question 2: Which style is better, and why?

Question 3: Are there any other styles?

I personally prefer hurdle style because it looks easier on the eyes and does not keep indenting the code to the right every time there's a new parameter. It allows intermittent code between checks, and it's neat, but it's also a little difficult to maintain (several exit points).

The first version of fence style quickly gets really ugly when adding parameters, but I suppose it's also easier to understand. While the second version is better, it can be broken accidentally by a future coder, and does not allow intermittent code between conditional checks.

ADTC
  • 709

2 Answers2

27

Your first “hurdle” style is superior in any respect.

  • It saves you many levels of indentation. This alone makes it much more straightforward to understand.
  • It's diff friendly: Adding another constraint doesn't require you to re-indent all your code.
  • Validation is located at one place – at the top of the function. With the other style, it is before and after the main code. Putting all validation code into one place reduces cognitive load.
  • Multiple exit points don't matter very much in a language with garbage collection. Striving for a single return is a relic from olden times where everything had to be deallocated manually.
amon
  • 135,795
7

Your example is specifically dealing with checking that the calling code is conforming to the contract. If that's the case then the hurdle style is typically cleaner and easier to read, usually because you will throw an exception so there's no expectation that there's some other piece of code lower down in the method that will miss being executed.

There is another case where you're decomposing the function into two parts (similar to pattern matching in functional languages). Consider the case of a recursive function:

int sum(int upTo)
{
    if(upTo<= 0) return 0;

    int previousSum = sum(upTo-1);
    return previousSum + upTo;
}

If you're only checking for stopping criteria, then the hurdle style is still typical.

There is another case where you have to do something else after your decision (this is less common):

int sum(int upTo)
{
    int result;
    if(counter <= 0) 
    {
        result = 0;
    }
    else
    {
        int previousSum = sum(upTo-1);
        result = previousSum + upTo;
    }

    Console.WriteLine("Intermediate sum result: {0}", result);

    return result;
}

In that case it's typical to use the fence style. Note that using the hurdle style here would produce a different result, because you either need to write the Console line twice or it won't spit out the line when the value is zero.

The style you choose communicates to the reader what type of function it is. If you find that the function or method becomes so long that it's not clear, then consider breaking it up into multiple functions to make it clearer.

Typically this should be changed to a hurdle style with a helper function:

int sum(int upTo)
{
    int result = sumHelper(upTo);
    Console.WriteLine("Intermediate sum result: {0}", result);
    return result;
}  

int sumHelper(int upTo)
{
    if(upTo<= 0) return 0;

    int previousSum = sum(upTo-1);
    return previousSum + upTo;
}

TL;DR - Hurdle is the most common, but choose the appropriate style for your specific situation and make sure your functions are short/simple enough to be obviously correct either way.

Scott Whitlock
  • 21,979