Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
9  
I personally would be able to understand the second one more than the first, but I seldom use finally blocks. –  Christian Mann Apr 28 '12 at 2:03
1  
return current = fetcher.fetchNext(current); // How about this, aka do you really need prefetching? –  scarfridge Apr 28 '12 at 8:33
1  
@scarfridge The example is about implementing Iterator, where you indeed need sort of prefetching in order for hasNext() to work. Try it yourself. –  maaartinus Apr 28 '12 at 12:37
1  
@maaartinus I'm aware of that. Sometimes hasNext() simply returns true e.g. hailstone sequence and sometimes fetching the next element is prohibitively expensive. In the latter case you should resort to fetching the element only on demand similar to lazy initialization. –  scarfridge Apr 28 '12 at 16:52
1  
@scarfridge The problem with common use of Iterator is that you need to fetch the value in hasNext() ('cause fetching it is quite often the only way to find out if it exists) and return it in next() just like the OP did. –  maaartinus Apr 28 '12 at 19:07
show 4 more comments

5 Answers

up vote 16 down vote accepted

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.

share|improve this answer
1  
+1 for the "clever" comment. That very accurately captures this example. –  GlenH7 May 3 '12 at 18:30
2  
The 'return and advance' action of next() is forced upon the OP by the unwieldy Java iterator interface. –  kevin cline May 6 '12 at 17:16
add comment

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.

share|improve this answer
2  
A try/catch/finally block might also add extra overhead, I'm not sure if javac or the JIT compiler would strip them out even if you're not throwing an Exception. –  Martijn Verburg Apr 28 '12 at 10:32
2  
@MartijnVerburg yeah, javac still adds an entry to the exception table and duplicates the finally code even if the try block is empty. –  Daniel Apr 28 '12 at 12:28
    
@Daniel - thanks I was too lazy to execute javap :-) –  Martijn Verburg Apr 28 '12 at 12:48
    
@Martijn Verburg: AFAIK, a try-catch-finallt block is essentially free as long as there's no actual exception thrown. Here, javac doesn't try and doesn't need to optimize anything as JIT will take care of it. –  maaartinus Nov 13 '12 at 15:02
    
@maaartinus - thanks that does make sense - need to read OpenJDK source code again :-) –  Martijn Verburg Nov 13 '12 at 15:30
add comment

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.

share|improve this answer
    
Aren't you actually providing a pretty good reason to avoid using finally in this way, then? I mean, as you say, the code ends up having unwanted consequences; even worse, you are probably not even thinking about exceptions. –  Andres F. Apr 28 '12 at 3:07
2  
The normal control flow speed is not significantly affected by the exception-handling constructs. The performance penalty is only paid when actually throwing and catching exceptions, not when entering a try block or entering a finally block in normal operation. –  yfeldblum Apr 28 '12 at 3:17
1  
@AndresF. True, but that's also valid for any cleanup code. For example, suppose a reference to an open stream is set to null by a buggy code; trying to read from it will raise an exception, trying to close it in the finally block will raise another exception. Besides, this is a situation where the state of the computation doesn't matter, you want to close the stream whether or not an exception occurred. There might be other situations like that as well. For this reason I'm treating it as a pitfall for the valid uses, rather than a reccomendation to never use it. –  mgibsonbr Apr 28 '12 at 3:22
    
There is also the problem that when both the try and finally block throw an exception, the exception in the try is never seen by anyone, but it is probably the cause of the problem. –  hstoerr Apr 30 '12 at 20:18
add comment

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.

share|improve this answer
    
+1, I would add that even if the language spec guarantees that the value is stored before it is returned, it pitentially goes against reader expectation, and thus should be avoided when reasonably possible. Debugging is twice as hard as coding... –  jmoreno Jan 11 '13 at 9:01
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.