Use while loop to avoid deeply nested if statements in java

Question

Hi I have written a little function like

public void foo(MyClassA paraA) {
    if (paraA == null) return;
    MyClassB paraB = doSomeStuff(paraA);
    if (paraB == null) return;
    MyClassC paraC = doMoreStuff(paraB);
    if (paraC == null) return;
    ....
}

The above fails fast and is nice to read (i.e. the intention to return on null values is clear). But now instead of simply returning, I want to do some error logging, so I changed to

public void foo(MyClassA paraA) {
    if (paraA == null) {doLog(); return;}
    MyClassB paraB = doSomeStuff(paraA);
    if (paraB == null) {doLog(); return;}
    MyClassC paraC = doMoreStuff(paraB);
    if (paraC == null) {doLog(); return;}
    ....
}

The above is also clean and easy to read, but I have to repeat doLog() a couple of times. So I change again to

public void foo(MyClassA paraA) {
    if (paraA != null) {
        MyClassB paraB = doSomeStuff(paraA);
        if (paraB != null) {
            MyClassC paraC = doMoreStuff(paraB);
            if (paraC != null) {
                ....
                return;
            }
        }
    }
    doLog();
}

The above calls doLog() only just once but I ended with some deeply-nested if statements, which are very ugly and hard to read. So how do I keep the same cleanliness like before and have doLog() just once? Note that returning something else rather than void for foo() is not allowed. And I also read that using try/catch as oppose to null check is an anti-pattern.

If I am to try, I want to write something like

public void foo(MyClassA paraA) {
    while(true) {
        if (paraA == null) break;
        MyClassB paraB = doSomeStuff(paraA);
        if (paraB == null) break;
        MyClassC paraC = doMoreStuff(paraB);
        if (paraC == null) break;
        ....
        return;
    }
    doLog();
}

The above fulfills all my needs(fail fast, clean, no nested if), but is the use of the while loop here an anti-pattern as the while loop here is never meant to run more than once?

@shree.pat18 havent thought about that far, why? May be you can give your suggestions on both with parameter or without, thank you. — user1589188
– user1589188, Commented Feb 13, 2015 at 7:07
If you can append to a string each time one of the variables is null, and then pass the resulting string to doLog once at the end, you wouldn't have to make repeated calls to it. — shree.pat18
– shree.pat18, Commented Feb 13, 2015 at 7:08
@shree.pat18 yes, that can be done but not related to my question. Which pattern you are talking about which calls doLog just once? If you are referring to the nested-if, I am against that. If you are referring to my while loop version, I have my question asking if using while loop an anti-pattern. — user1589188
– user1589188, Commented Feb 13, 2015 at 7:12
because of the return; it will not reach to dolog() method hope that is fine with you. — someone
– someone, Commented Feb 13, 2015 at 7:13

David Ehrmann · Accepted Answer · 2015-02-13 07:16:45Z

3

Java has a nifty labeled break construct that might help you, here.

public void foo(MyClassA paraA) {
    block: {
        if (paraA == null) { break block; }
        MyClassB paraB = doSomeStuff(paraA);
        if (paraB == null) { break block; }
        MyClassC paraC = doMoreStuff(paraB);
        if (paraC == null) { break block; }
        ...
        return;
    }

    doLog();
}

If you used polymorphism better, you could do this:

public void foo(MyInterface para) {
    while (para != null) {
        para = para.doStuff();
    }
    doLog();
}

If you absolutely can't use polymorphism like that, use a dispatcher.

But I've seen this before, and it looks like a state machine. Give "java enum state machine" a search. I have a feeling that's what you're really trying to do.

edited Feb 13, 2015 at 7:16

answered Feb 13, 2015 at 7:11

David Ehrmann

7,6143 gold badges35 silver badges46 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

user1589188 Over a year ago

I dont know you can use break without being inside a for or while loop! Thats interesting, let me try this. If it works, most likely I will pick yours as the answer.

user207421 Over a year ago

See JLS #14.15.

David Ehrmann Over a year ago

@user1589188 Thanks, but please think about the polymorphic approach for this, even if you don't end up using it. It's a good pattern to know.

user1589188 Over a year ago

Yup, tried and it works. The only remaining question is, I feel this labelling sort of like goto. Isn't that another anti-pattern? Can't do poly-morphing doStuff() as I need to pass in some parameters at different stage.

David Ehrmann Over a year ago

It's not really any different from multiple returns from a single function...which is a bit like goto, but the abuses of goto were so much worse than multiple returns, break, and continue ever were. There are definitely people who don't like multiple returns, but I'd say it's really more of a personal preference at this point.

|

someone · Accepted Answer · 2015-02-13 07:08:04Z

1

Do you think that this is clean

public void foo(MyClassA paraA) {

    MyClassB paraB = paraA != null?doSomeStuff(paraA):null;
    MyClassC paraC = paraB != null?doMoreStuff(paraB):null;

     if (paraC != null) {
         ....

     }

     doLog();
}

answered Feb 13, 2015 at 7:08

someone

6,5728 gold badges43 silver badges63 bronze badges

3 Comments

Cole Tobin Over a year ago

Even better, put the null check in doMoreStuff

user1589188 Over a year ago

This is clean, but does not fail fast. Once you know paraA is null, you should go to doLog() directly, so yours is not optimal where you have to eval some more condition checks before reaching doLog()

Has QUIT--Anony-Mousse Over a year ago

null checks are as fast as it gets. Leave it to hotspot to optimize them with a jmp.

Has QUIT--Anony-Mousse · Accepted Answer · 2015-02-14 10:57:01Z

IMHO your second code snippet is what ypu should do.

Do not try to make your code short. This is an antipattern.

if (a==null) {
  log("Failed in step a");
  return;
}
B b = a.doSomething();

is very fast to read and understand. You save nothing by compressing this code. Zero. Nada. Leave it to Hotspot VM, and focus on making code understandable. "if null then log return" is a classic, well understood and accepted pattern.

It had become popular to try to make code "readable" with lambda antipatterns like this:

B b = ifNullLog(a, () -> a.doSomething())

where

T ifNullLog(Object guard, Function<T> func) {
  if (guard == null) { doLog(); return null; }
  return func.run();
}

but IMHO this is a total antipattern. In fact, it is a best practise to even require braces for every if, else, for, while to make it easy to insert such a log statement without risking to break the code.

Code like your first snippet:

if (a == null) return;

are dangerous. Look at various bugs like Apples SSL disaster If someone adds doLog without noticing the missing brackets, the function will always return null. The apple SSL bug (or was it heartbleed?) esdentially was a

if (a==null)
  return;
  return;
B b = a.doSomething();

See how subtle the bug is? You Java compiler will fortunately warn you if it's about unreachable code - it will not necessarily warn you otherwise... by always using brackets and well formatted code such bugs can easily be avoided. Format code to avoid errors, not for aestetics.

It is also well acceptable to use return codes. Just don't make success default (see heartbleed again).

Code c = execute(a);
if (c != Code.SUCCESS) {
  doLog(c);
  return;
}

where

Code execute(A a) {
  if (a == null) { return Code.FAILED_A_NULL; }
  B b = a.doSomething();
  if (b == null) { return Code.FAILED_B_NULL; }
  ...
  return Code.SUCCESS;
}

A classic use case of "return", another good pattern.

Collectives™ on Stack Overflow

Use while loop to avoid deeply nested if statements in java

3 Answers 3

6 Comments

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

6 Comments

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related