In Java you sometimes have complex exceptions whose creation requires more than just a call to new
. In those cases, I like to use a helper method to separate the the construction of the exception from the code which might throw the exception. (In fact my taste is to use such a helper anytime the exception is not ready in a single line of code. Sadly, IOException
is such an exception when it has a cause: it is missing the full set of constructors that other descendents of Exception
have.)
For such times which of these two snippets is preferred?
public void work(final Queue<PieceOfWork> workQueue) throws IOException { if (!doWork(workQueue)) throw createWorkException(workQueue); } private WorkException createWorkException(final Queue<PieceOfWork> workQueue) { final WorkException e = new WorkException(getJobId()); for (final PieceOfWork piece : workQueue) e.addPieceOfWork(piece); return e; }
Or:
public void work(final Queue<PieceOfWork> workQueue) throws WorkException { if (!doWork(workQueue)) throwWorkException(workQueue); } private void throwWorkException(final Queue<PieceOfWork> workQueue) throws WorkException { final WorkException e = new WorkException(getJobId()); for (final PieceOfWork piece : workQueue) e.addPieceOfWork(piece); throw e; }
Until recently I thought there was little difference between them but have changed my mind. Why? The first case is easy for static analysis tools to identify that there are two code paths, one of which throws an exception. The second case requires that the tool descend into throwWorkException
and work out that one of the code paths throws an exception.
Although this is not much to ask of many of the more sophisticated static analysis tools, it is too much to ask of most of the tools bundled with common Java editors. IntelliJ IDEA, for example, does not pick up on this as it analysis your code as you type: it makes the sensible tradeoff of performance for completeness.
In fact, shallow static analysis is the reason javac
flags this as an error:
public boolean checkCondition() throws SomeException { if (!cannotProceed()) throwSomeException(); else return true; // Missing return statement }
Sad code, yes, but not uncommon, especially when the branches get very complicated so that it is not so obvious that they could be simplified.
Lastly, keeping the throw
statement as close as possible to where the condition failed helps the most common static analysis tool of all: the human eye. When quickly browing others code, it is easy to miss the true code flow without the helpful throw
keyword staring one right in the face.
2 comments:
I have run into this problem also. Unfortunately the obvious did not occur to me that the helper method could simply return the exception instead of throwing it. Perhaps if I had been pairing, my pair would have seen it. I also agree that it is more intention revealing to "the human eye".
I've used the first technique coupled with Throwable.fillInStackTrace so that exception "originates" from the throwing method.
public void work(final Queue workQueue)
throws IOException {
if (!doWork(workQueue))
throw (IOException)createWorkException(workQueue).fillInStackTrace();
}
Riad
Post a Comment