Loop continuing even after hitting return statement

203 Views Asked by At

I have the following method where I wan to keep checking if a nested exception matches an IndexOutOfBoundsException unless the nested exception is the same as the previous one.

It seems to do the right thing where in my test, the first exception is of type NullPointerException thus moving on to the next. The next exception is as expected, an IndexOutOfBoundsException.

When this happens I want to return true which I expect to break me out of the loop. It seems to be happening as expected where I do land on 'return true'. But after that, the loop keeps going.

What am I missing. How is it keeping on going even after returning true?

public boolean test(Throwable throwable) {

    do {
        if(throwable instanceof IndexOutOfBoundsException) {
            return true; // I do land here thus expecting to get out of loop and this method.
        }
        this.test(throwable.getCause());
    } while (throwable.getCause() != throwable);

    return false;
}

The test against it simulating nested exception.

@Test
void testWithNestedException() {
    NullPointerException nullPointerException = new NullPointerException();
    IndexOutOfBoundsException indexOutOfBoundsException = new IndexOutOfBoundsException();
    Throwable nestedException = nullPointerException.initCause(indexOutOfBoundsException);
    assertTrue(someClass.test(nestedException));
}
3

There are 3 best solutions below

4
Mureinik On BEST ANSWER

You are mixing recursion with looping. Here, a simple loop that updates the exception being tested should do the trick:

public boolean test(Throwable throwable) {
    Throwable t = throwable;
    do {
        if (throwable instanceof IndexOutOfBoundsException) {
            return true;
        }
        t = t.getCause();
    } while (t.getCause() != t);

    return false;
}
2
TomStroemer On

You are creating recursion with this call and don't use the return code from this call.

this.test(throwable.getCause());

I think you wanted to do:

throwable = throwable.getCause();
1
Stephen C On

As @Mureinik points out, you are mixing recursion and iteration and doing neither correctly. A (correct) recursive version would be:

public boolean test(Throwable throwable) {
    if (throwable == null) {
        return false;
    } else if (throwable instanceof IndexOutOfBoundsException) {
        return true;
    } else {
        return test(throwable.getCause());
    }
} 

In my opinion, a recursive version is easier to understand than an iterative one, but others may disagree.

With the recursive version, there is the theoretical problem that sufficiently deeply nested exceptions could lead to a stack overflow. Getting that to happen in practice would require some rather contrived (i.e. unrealistic) code, so it should be safe to ignore this.