9

This question already has an answer here:

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

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

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.

marked as duplicate by Kilian Foth, Telastyn, Karl Bielefeldt, user22815, GlenH7 Sep 9 '14 at 16:57

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 1
    I voted as duplicate, though this question seems better formed than the other. – Telastyn Sep 9 '14 at 14:53
  • the labeled block is an indicator that you can extract the tests to a function and return true/false in the previous style – ratchet freak Sep 9 '14 at 14:55
  • I would really be thankful if this isn't closed as duplicate. While I appreciate the link, it doesn't exactly satisfy the question here. @ratchetfreak yes, I understand. That's why I mentioned "cannot [or should not] be moved to a smaller private method" but normally it shouldn't be the case and I should be able to extract the tests. Let's not focus on that. – ADTC Sep 9 '14 at 14:59
  • And a quick comment about #3 - I use param = param ?? default; often in C#, which is hurdle style, though with different semantics. – Telastyn Sep 9 '14 at 15:02
  • @Telastyn That's good. Well, different languages have different code semantics but style practices can be carried over. I'm using Java here and the closest to that would be param = (param==null ? default : param) which is uglier than if (param==null) param = default :)... Not too sure about Java 8 lambda expressions though. – ADTC Sep 9 '14 at 15:08
20

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.
  • 7
    I will sometimes take this a step further and have a public function that performs precondition checks, then delegates to a private or protected function that does the actual work and assumes the first method already validated everything. Helps keep each function lean, laser-focused, and more concise. – user22815 Sep 9 '14 at 15:23
  • @user22815 I agree. This is where I find languages with preconditions, postcondition and invariants features interesting: you actually have to fully express those when declaring methods, and leave the actual implementation free of inputs/outputs/invariant checks. Contract has a formal existence into code (compilation time, and even at execution time) and is human readable too : no more tons of code documentation explaining all the tricks – LoneWanderer Mar 10 at 23:31
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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.