Java Stream Exception Handling

Question

I find this piece of Java code really ugly and cumbersome. How can I refactor it to be more clean and easier to read?

ByteArrayOutputStream out = null;
try {
 out = new ByteArrayOutputStream();
 workbook.write(out);
 return out.toByteArray();
} catch (Exception e) {
 e.printStackTrace();
} finally {
 try {
 if (out != null) {
 out.close();
 }
 } catch (IOException e) {
 e.printStackTrace();
 }
}
return null;

• fge: Don't catch Exception! This also catches RuntimeException therefore all unchecked exceptions. Catch RuntimeException first then rethrow.

Answer 1

From java 7 on you can use the try-with-resources syntax :

try (ByteArrayOutputStream out = new ByteArrayOutputStream()) {
 workbook.write(out);
 return out.toByteArray();
} catch (Exception e) {
 e.printStackTrace();
}
return null;

Which handles the closing of the Stream, and any excptions that may throw, for you.

In earlier versions of Java, you're pretty much stuck with what you have.

Edit:

It is worth noting that in case you get an exception on the close() method, it is still the original exception that is being thrown. The exception on the close() method is then added to that exception as a suppressed exception. You can get at the suppressed exceptions by calling java.lang.Throwable#getSuppressed

• user11153: But it's worth noting that "Closing a ByteArrayOutputStream has no effect."
• WernerCD: +1 nice to know Java has the C# equivalent of using, which closes/disposes when it goes out of scope.

Answer 2

In pre-java 7 (without the try with resources) I find it easier to nest the try-finally in a try-catch:

try{
 ByteArrayOutputStream out = new ByteArrayOutputStream();
 try{
 workbook.write(out);
 return out.toByteArray();
 }finally{
 out.close();
 }
}catch(IOException e){
 //handle
}

This way there is only 1 point where you need to handle the exception at the cost of an extra indentation level.

• fgb: One problem here is that if workbook.write throws, then you get a null pointer exception in the finally block which hides the original error.
• ratchet freak: @fgb no because out is initialized before the try, and workbook.write can't change it back to null
• fgb: Right, sorry. There's still a chance that out.close could throw a different exception, but probably not with a ByteArrayOutputStream.

Answer 3

You are catching the exceptions for three purposes:

1. Close the stream to prevent a resource leak,
2. Print a stack trace,
3. Return null instead of a byte array.

Of those three consequences, only the first is what I would consider good practice, but even that turns out to be unimportant in the special case of a ByteArrayOutputStream:

Closing a ByteArrayOutputStream has no effect. The methods in this class can be called after the stream has been closed without generating an IOException.

So, your whole finally clause can be dropped!

Now, we can focus on catch (Exception e). Pokémon clauses should be viewed with great suspicion. What kinds of exceptions might possibly be thrown by workbook.write(out)? IOException? MalformedWorkbookException? You should list exactly what you expect, so as not to swallow exceptions that you didn't intend to handle.

Working with the assumption that IOException should be impossible when writing to a ByteArrayOutputStream, and that the only other remotely probable kind of exception is something like a MalformedWorkbookException (which really indicates a programming error in your own code rather than a condition triggered by user-controllable conditions), I would recommend re-thowing the exception. However, since it would be cumbersome to declare that your code throws IOException even though it is logically impossible, I suggest smuggling it out by wrapping it in something else.

try (ByteArrayOutputStream out = new ByteArrayOutputStream()) {
 workbook.write(out);
 return out.toByteArray();
} catch (MalformedWorkbookException | IOException e) {
 throw new AssertionError("writing workbook", e);
}

• fgb: In the last example, could you re-throw the exception as some kind of runtime exception instead? Assertions are often disabled which would result in losing any trace to the original error.
• 200_success: @fgb I had considered that. But then you would have to declare at least an IOException on the method even though it is logically impossible. I suppose you could smuggle out an IOException by wrapping it in a RuntimeException of some sort, but I opted for simplicity.
• maaartinus: I'd really replace assert false by throw new RuntimeException(e) as the former provides no exception chaining (which could be a problem with a longer code block). This way you'd get rid of return null too, which is good as the method is not supposed to do it and should be declared as such (if possible) and then findbugs and other tools would complain.
• 200_success: @maaartinus You're right. I've changed my mind.

Answer 4

I do agree with @bowmore But for someone who is stuck with java 6 and below. Simply create a utility class which will flush and close output stream for you.

public static releaseResource(OutputStream out/Writer out){
 if(out==null)
 return;
 if(out.isClosed())
 return;
 try {
 out.flush();
 out.close();
 } catch (IOException e) {
 e.printStackTrace();
 }
}

Create two such overloaded method for OutputStream and Writer. Preferably call this method in finally clause.

You can put it in a utility class or in some base class DAO class and extend it.

This will be some thing like this.

ByteArrayOutputStream out = null;
try {
 out = new ByteArrayOutputStream();
 workbook.write(out);
 return out.toByteArray();
} catch (Exception e) {
 e.printStackTrace();
} finally {
 DBUtil.releaseResource(out);
}

Answer 5

Since the close-method of ByteArrayOutputStream does nothing, you can do this:

ByteArrayOutputStream out = new ByteArrayOutputStream();
workbook.write(out);
return out.toByteArray();

Since catching all exceptions and swallowing them is a bad practice, I've omitted that code as well.