Is use of finally clause for doing work after return bad style/dangerous?

Question

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.

Answer 1

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.

Answer 2

Purely from a style standpoint, I think these three lines:

T current = next;
next = fetcher.getNext();
return current;

... are both more obvious and shorter than a try/finally block. Since you're not expecting any exceptions to be thrown, using a try block is just going to confuse people.

Answer 3

That would depend on the purpose of the code inside the finally block. The canonical example is closing a stream after reading/writing from it, some sort of "cleanup" that must be always done. Restoring an object (in this case an iterator) to a valid state IMHO also counts as cleanup, so I see no problem here. If OTOH you were using return as soon as your return value was found, and adding a lot of unrelated code in the finally block, then it would obscure the purpose and make it all less understandable.

I don't see any problem in using it when "there are no exceptions involved". It's very common to use try...finally without a catch when the code can only throw RuntimeException and you don't plan on handling them. Sometimes, the finally is just a safeguard, and you know for the logic of your program that no exception will ever be thrown (the classic "this should never happen" condition).

Pitfalls: any exception raised inside the try block will make the finally bock run. That can put you in an inconsistent state. So, if your return statement were something like:

return preProcess(next);

and this code raised an exception, fetchNext would still run. OTOH if you coded it like:

T ret = preProcess(next);
next = fetcher.fetchNext(next);
return ret;

then it would not run. I know you're assuming the try code can never raise any exception, but for cases more complex than this how can you be sure? If your iterator were a long-lived object, that would continue existing even if an unrecoverable error happened in the current thread, than it would be important to keep it in a valid state at all times. Otherwise, it doesn't really matter much...

Performance: it would be interesting to decompile such code to see how it works under the hood, but I don't know enough of the JVM to even take a good guess... Exceptions are usually "exceptional", so the code that handles them don't need to be optimized for speed (hence the advice to never use exceptions in the normal control flow of your programs), but I dunno about finally.

Answer 4

The title statement: "...finally clause for doing work after return..." is false. The finally block happens before the function returns. That is the whole point of finally in fact.

What you are abusing here is the order of evaluation, where the value of next is stored for returning before you mutate it. It is not common practice and in my opinion wrong as it makes you code non-sequential and therefore much harder to follow.

The intended use of finally blocks is for exception handling where some consequence must happen regardless of exceptions being thrown e.g. cleaning up some resource, closing DB connection, closing a file/socket etc.

Answer 5

I would be really carefull, because (in general, not in your example) part in try can throw exception, and if it is thrown out, nothing will be returned and if you actualy inteded to execute what is in finally after return, it will execute when you did not want it to.

And other can of worm is, that in general, some usefull action in finally can throw exceptions, so you can end with really messy method.

Because of these, someone reading it must think about these problems, so it is harder to understand.