Java Code Quality – Is Using Finally Clause After Return Bad?

code-qualityexceptionsjavareadability

As part of writing an Iterator, I found myself writing the following piece of code (stripping error handling)

public T next() {
  try {
    return next;
  } finally {
    next = fetcher.fetchNext(next);
  }
}

finding it slightly easier to read than

public T next() {
  T tmp = next;
  next = fetcher.fetchNext(next);
  return tmp;
}

I know it's a simple example, where the difference in readability may not be that overwhelming, but I'm interested in the general opinion as to whether it is bad to use try-finally in cases like this where there are no exceptions involved, or if it is actually preferred when it simplifies the code.

If it's bad: why? Style, performance, pitfalls, …?

Conclusion
Thanks you all your answers! I guess the conclusion (at least for me) is that the first example might have been more readable if it was a common pattern, but that it isn't. Therefore the confusion introduced by using a construct outside of it's purpose, along with possibly obfuscated exception flow, will outweigh any simplifications.

Best Answer

Personally, I'd give a thought to the idea of command query separation. When you get right down to it, next() has two purposes: the advertised purpose of retrieving the element next, and the hidden side effect of mutating the internal state of next. So, what you're doing is performing the advertised purpose in the body of the method and then tacking on a hidden side effect in a finally clause, which seems... awkward, though not 'wrong', exactly.

What it really boils down to is how understandable the code is, I'd say, and in this case the answer is "meh". You're "trying" a simple return statement and then executing a hidden side effect in a code block that's supposed to be for fail-safe error recovery. What you're doing is 'clever', and 'clever' code often induces maintainers to mumble, "what the... oh, I think I get it." Better for people reading your code to mumble "yep, uh-huh, makes sense, yes..."

So, what if you separated state mutation from accessor calls? I'd imagine the readability issue you're concerned with becomes moot, but I don't know how that affects your broader abstraction. Something to consider, anyway.

Related Solutions

Why Use Try Finally Without a Catch Clause?

It depends on whether you can deal with the exceptions that can be raised at this point or not.

If you can handle the exceptions locally you should, and it is better to handle the error as close to where it is raised as possible.

If you can't handle them locally then just having a try / finally block is perfectly reasonable - assuming there's some code you need to execute regardless of whether the method succeeded or not. For example (from Neil's comment), opening a stream and then passing that stream to an inner method to be loaded is an excellent example of when you'd need try { } finally { }, using the finally clause to ensure that the stream is closed regardless of the success or failure of the read.

However, you will still need an exception handler somewhere in your code - unless you want your application to crash completely of course. It depends on the architecture of your application exactly where that handler is.

Java – Difference Between Code in Finally Clause and After Catch Clause

The finally {} block is always called (unless you call exit() before finally is reached - but you won't be doing that most of the time).

The purpose of finally is to clean up any resources allocated in the try {} block, even if something was thrown and it hit the catch{} block.

Consider the code:

public class Hello {

    public static void main(String[] args) {
        hello();
    }

    public static void hello() {
        try {
            System.out.println("hi");
            return;
        } catch (RuntimeException e) {
        } finally {
            System.out.println("finally");
        }
    }
}

While there isn't any resource allocation in this, just pretend there is. This will print out:

hi
finally

If you had opened a file in the try block, the finally is the place to clean it up. If you had opened one file, and then tried to open another one (that threw an exception), the finally is the place to clean that up.

If there was an uncaught exception by the catch block, then the finally would run, and the exception would get passed on up.

Putting out.close() in the catch and the finally is redundant - because it will be run in the finally block - on a caught exception or successful completion of the try.

With the existing code, the following code paths are possible:

No exception:
1. check() - no exception thrown
2. finally { } - out.close
Caught exception:
1. check() - MissingParamException thrown
2. catch { } - stuff and out.close
3. finally { } - out.close
Uncaught exception:
1. check() - NPE thrown
2. finally { } - out.close

You should be able to see from this, the finally is always called, and in the caught exception, out.close() is invoked twice.

The associated spec is §14.20.2 in Java SE 7 language specification.