Don’t Extract Everything Into a Method

Every now and then, I tweet something like this, just to piss off some clean coders:
Apart from the obvious trolling factor (why can’t I ever resist?), I do think there’s something thought provoking in such a tweet. First off, given how rare break and continue statements are in Java code, many people probably don’t know that you can break out of an if-else block, or any labelled block, for that matter. Secondly, we’ve been following cargo cults about clean code so much that we completely forget about some more obscure language features of our favourite languages that would be so handy in edge cases. Like these ones!

Breaking out of loops

Most people know how this works:

for (int i = 0;; i++)

    // Stop at 42
    if (i == 42)
        break;

    // Ignore even numbers
    else if (i % 2 == 0)
        continue;

    // Main action
    else
        System.out.println("i = " + i);

Of course, this code is bogus just to illustrate the syntax. A much simpler, refactored, equivalent version is this:

// Stop criteria in loop, duh
for (int i = 0; i < 42; i++)

    // Inverse if and else for even numbers
    if (i % 2 != 0)
        System.out.println("i = " + i);

This refactoring was obvious and trivial because the loop is so simple. But sometimes it isn’t, or sometimes the break or continue statement makes a loop just simpler to read (“simplicity” like “beauty” is in the eye of the beholder, of course). Parsers are the most obvious example, but also, for example, consider jOOQ’s AbstractRecord.compareTo() method, which contains this loop:

for (int i = 0; i < size(); i++) {
    final Object thisValue = get(i);
    final Object thatValue = that.get(i);

    if (thisValue == null && thatValue == null)
        continue;
    else if (thisValue == null)
        return 1;
    else if (thatValue == null)
        return -1;
    else {
        // ... Actually interesting comparisons
    }
}

// If we got through the above loop, the two records are equal
return 0;

jOOQ Records are compared with each other like SQL Records (if they are comparable, i.e. of equal degree): From the the first attribute to the last. The interesting bit here is the continue statement, that skips to the next attribute, if both attributes are null. Of course, if you are a feverish hater of such explicit, goto-style control flow, you would probably refactor this to something “much better”, namely:

for (int i = 0; i < size(); i++) {
    final Object thisValue = get(i);
    final Object thatValue = that.get(i);

    if (!(thisValue == null && thatValue == null)) {
        if (thisValue == null)
            return 1;
        else if (thatValue == null)
            return -1;
        else {
            // ... Actually interesting comparisons
        }
    }
}

// If we got through the above loop, the two records are equal
return 0;

Drawbacks:
  • There’s one more level of indentation
  • The if-else branch series is no longer “regular”, i.e. some conditions lead to quite different control flow than others
  • This worked fine for a single continue statement, but we might complicate this loop and add a second continue statement, or better: A nested loop that also contains break or continue statements…
With loops, the choice of using break or continue is sometimes “obvious”. Refactoring the imperative programming style to something more “elegant” might take quite a while of thinking, introduce regressions, and (ghasp) make the code more complex and less readable. Yes, I said it. Sometimes, imperative style is simpler (not better, just simpler). Of course, this isn’t necessarily true in this particular example, so don’t haunt me ;)

Breaking out of an if

While breaking out of loops (or continuing them from in the middle) is somewhat common in complicated loops, breaking out of an if is certainly less common. In fact, you can break out of any labeled statement in Java (label-less breaks are possible with loops only):

// No-op
label1:
break label1;

// More interesting, Russian Roulette:
label2: {
    System.out.println("Pulling trigger");

    if (Math.random() < 0.16) {
        System.out.println("Click");
        break label2;
    }

    System.out.println("Whew");
}

So, why not break out of an if-else?

public class FunWithBreak {
    public static void main(String[] args) {
        label:
        if (args.length > 0) {
            System.out.println("We got args!");

            for (String arg : args)
                if ("ignore-args".equals(arg))
                    break label;

            for (String arg : args)
                System.out.println("Arg : " + arg);
        }
        else {
            System.out.println("No args");
        }

        System.out.println("End of program");
    }
}

Now run this program from the commandline
> java FunWithBreak
No args
End of program

> java FunWithBreak a b c
We got args!
Arg : a
Arg : b
Arg : c
End of program

> java FunWithBreak a b c ignore-args
We got args!
End of program
It’s kind of neat, no? The break in the middle of the if statement. It makes sense because there’s an early abort condition for the if-block, and only for the if-block.

Alternatives

Someone responded to my original tweet, that I should refactor the code and factor out a method for the if block, which would allow for replacing the break by return. Here’s how that would work:

public class FunWithBreak {
    public static void main(String[] args) {
        if (args.length > 0)
            nameThisLater(args);
        else
            System.out.println("No args");

        System.out.println("End of program");
    }

    // TODO find a meaningful name for this method later
    private static void nameThisLater(String[] args) {
        System.out.println("We got args!");

        for (String arg : args)
            if ("ignore-args".equals(arg))
                return;

        for (String arg : args)
            System.out.println("Arg : " + arg);
    }
}

Now, that certainly looks more common, which doesn’t mean it’s better. We’ve now factored out a useless method that is called only exactly once, and that is inlined (hopefully) by the JIT to become the original code again. The advantage is that our original main() method got a bit simpler to read (once we’ve found a meaningful name for the method), but again, simplicity is in the eye of the beholder. But the price we pay here is that we have to pass all local state as method arguments. Simple in this case (single local variable args), but that can become quite complex, especially when we have local mutable collections that need to be passed around. Besides, others might argue that one shouldn’t return from the middle of a method. They are proponents of “single-return-statement methods”. Sure, why not. Here’s how that would look:

public class FunWithBreak {
    public static void main(String[] args) {
        if (args.length > 0) {
            ifBlock(args);
        }
        else {
            System.out.println("No args");
        }

        System.out.println("End of program");
    }

    private static void ifBlock(String[] args) {
        System.out.println("We got args!");

        boolean ignoreArgs = false;
        for (String arg : args) {
            if ("ignore-args".equals(arg)) {
                ignoreArgs = true;
                break; // We need break again!
            }
        }

        if (!ignoreArgs)
            for (String arg : args)
                System.out.println("Arg : " + arg);
    }
}

Is it really better? I don’t know. I’ll just stick with the original break-out-of-if.

Bonus

Surprise your coworkers with an even more unusual placement of your label. Between the if statement and the ensuing block. After all, the curly braces are just an ordinary block, not part of the if statement

public class FunWithBreak {
    public static void main(String[] args) {
        if (args.length > 0) label: {
            System.out.println("We got args!");

            for (String arg : args)
                if ("ignore-args".equals(arg))
                    break label;

            for (String arg : args)
                System.out.println("Arg : " + arg);
        }
        else {
            System.out.println("No args");
        }

        System.out.println("End of program");
    }
}

Second bonus

Now that we’ve gotten all hooked on imperative control flow… What does this print?

for (int i = 0; i < 3; i++) {

    hmmm:
    try {
        System.out.println("Try      : " + i);
        if (i % 2 == 0)
            break hmmm;
        else
            throw new RuntimeException("Hmmm");
    }
    finally {
        System.out.println("Finally  : " + i);
        if (i < 2)
            continue;
    }

    System.out.println("Loop end : " + i);
}

Solution (select text below)
Try      : 0
Finally  : 0
Try      : 1
Finally  : 1
Try      : 2
Finally  : 2
Loop end : 2

16 thoughts on “Don’t Extract Everything Into a Method

  1. Lukas, Lukas, Lukas. Have you gone off your meds again? Come now lad, pull yourself together, or we’ll have to send the Ghost of Dykstra Past to visit you. :-)

    I won’t rise to the bait, except to say that anyone who did this in our shop would get a trip to the woodshed.

      1. Chuckle. Indeed!

        That being said, there is value in occasionally busting up our icons, and making us think about why we do the things the way we do. Full marks for that!

  2. I’ve been writing Java code for 14 years and I didn’t even know Java had labels! Anyway I looked up your blog because I miss JOOQ. I haven’t been working on a project that uses it :(

    1. Well, I’m glad you know them now :) (just don’t put them everywhere – some people’s eyes might start twitching…)

      Time to introduce jOOQ to new projects, then!

  3. hihi :-)

    Break is also useful, because “continue” in a forEach-loop won’t compile, but the following will:

    Arrays.asList(“a”, “b”).forEach(s -> {
      x: {
        if (s.equals(“a”)) break x;
        System.out.println(s);
      }
    });
    

    (I do hope the for<matting comes out right. I wish all blogs would support HTML or Markdown in their comments.)

    1. Very cunning, indeed ;)

      (I do hope the formatting comes out right. I wish all blogs would support HTML or Markdown in their comments.)

      The HTML element is called <pre>. Fixed it for you

  4. It does not really matter what happens in a local scope. It is more important to have a clear API (+ an exhaustive test-suite). Implementations are exchangeable.

    1. True. And sometimes, extracting a method risks increasing the API with things that shouldn’t be in the API. Oh, I wish Java had local functions :)

  5. I knew about label, but to be honest I don’t think I ever used them. I think there an another way to do it, which I think is more readable:

    public class FunWithBreak {
        public static void main(String[] args) {
            if (args.length > 0) label: {
                System.out.println("We got args!");
    
                if (!ignoreArguments(args)) {
                    for (String arg : args)
                        System.out.println("Arg : " + arg);
                }
            }
            else {
                System.out.println("No args");
            }
    
            System.out.println("End of program");
        }
    
        private static boolean ignoreArguments(String[] args) {
            for (String arg : args)
                if ("ignore-args".equals(arg))
                    return true;
            return false;
        }
    }
    
      1. You can argue that we have one more method (which could be avoided), but from my point of view is removing noise form the main method. I don’t have to check what is the logic for the ignoring arguments, if is one param, or two and what should be that special param value. I only know that I check if I should ignore the arguments or not. This means that, while reading the code, I can focus on what is happening in the main method, where the important logic is.
        Second argument would be that I in case of a more complex code, I don’t have to use labels. In this example it make sense to use it, but in real life usually the code is a bit more complex, which means that there is good chance that “label:” and “break label” are not on the same screen. This means that I would have to scroll up and down to see where the label is defined, interrupting my train of thoughts
        As I wrote on the original comment, this is why *I think* is more readable, but as you point it out in the simplicity and readability is “in the eye of the beholder”. Another factor which we should consider when we have to write the code is our team: do all of team members know what labels are? if yes, what do they think about them? which option would be easier for them to understaind/maintain?

        1. IDEs help you collapse blocks just like methods. I really don’t see that much of a difference between a labelled statement / block and a method, except that we’re more used to the latter than the former.

          You say “I don’t have to use labels”. Why yes you do. Your method name is a “label”. The only difference is that you semantically encourage a new stack frame (as long as the JIT doesn’t inline it, anyway), which is of no specific use.

          If “label:” and “break label” aren’t on the same screen, then by consequence, your refactored method declaration and the relevant “return” statement wouldn’t be on the same screen either.

          The team is certainly capable of learning 1-2 “new” language features. I wouldn’t worry about them too much.

          Anyway, you’re wasting your time :) I didn’t argue to completely avoid methods. I said, *occasionally*, don’t extract into a method, but use a labelled statement to break out of it. I think everyone can handle this *occasionally* and move on with their train of thoughts.

    1. Sure, you could argue this way. But then, you might outsource arg validation into a new method, and that method would do *only* arg validation, and still the article’s claims would hold true (i.e. occasionally preferring the less common syntax rather than extracting *yet another* method from the arg validating method)

Leave a Reply to Charles RothCancel reply