So what exactly *is* the intention of this method? IMHO, if an IOException was thrown when some stream was closed, it was probably for a reason, and probably something is broken, and probably the user should find out about it - and fix it - rather than have the problem be permanently suppressed from view. Maybe an exception closing a stream is harmless, but Ant surely doesn't know this; it could mean the last few Kb did not get flushed to disk, for example. E.g. I often see in Ant code like this (roughly adopted from ChangeLogTask.writeChangelog):
1.
My intention was only to uniform the codebase around the helper methods that were presumably created for this sort of task. If you originally swallow the exception (especially IOException - which I agree should be reported somewhere, but currently aren't), but with a finally block that first tests for null and then tries to close() stream/reader - and this can be replaced by one line FileUtils.close(), then surely this is more readable?
2.OutputStream os = null; try { os = new FileOutputStream(...); printStuffToStream(os); } catch (IOException e) { throw new BuildException(e); } finally { FileUtils.close(os); }
This follows the standard that I was aware of
1 try to do something
2 catch any exceptions and handle them gracefully
3 finally ensure that the program is in a state where we can continue or shutdown without breaking anything else
To my mind this is both harder to follow the flow of, and less robust than, the straightforward version:3.
try { OutputStream os = new FileOutputStream(...); try { printStuffToStream(os); } finally { os.close(); } } catch (IOException e) { throw new BuildException(e); }
when does finally get called? After the inner try, or at the end of the outer try block? Reading the spec only says that the VM honours the fact that finally will always be called before returning back up the call tree. Certain VM's could handle this differently (although I'm pretty sure they'd all just execute the inner stuff first).
IMHO this structure is less clear than the one above.
Here it is obvious than any IOException's get reported, and that an OutputStream will always be closed if it ever got created. You don't need any special utility method, or any dummy initial null value. You can even make the local var final if that floats your boat.4.
The utility method is designed to swallow the exception and not report it. So the rational of these changes is that you care about catching IOExceptions in the general body of the code (FileNotFound etc etc), but you don't want to report an exception when you're trying to clean up in the finally method, as any exceptions would have already been reported and indeed the stream etc would probably have been closed anyway, the finally is simply used as a sanity check.
for example
private Class findClassInComponents(String name)
throws ClassNotFoundException {
// we need to search the components of the path to see if
// we can find the class we want.
InputStream stream = null;
String classFilename = getClassFilename(name);
try {
Enumeration e = pathComponents.elements();
while (e.hasMoreElements()) {
File pathComponent = (File) e.nextElement();
try {
stream = getResourceStream(pathComponent, classFilename);
if (stream != null) {
log("Loaded from " + pathComponent + " "
+ classFilename, Project.MSG_DEBUG);
return getClassFromStream(stream, name, pathComponent);
}
} catch (SecurityException se) {
throw se;
} catch (IOException ioe) {
// ioe.printStackTrace();
log("Exception reading component " + pathComponent
+ " (reason: " + ioe.getMessage() + ")",
Project.MSG_VERBOSE);
}
}
throw new ClassNotFoundException(name);
} finally {
try {
if (stream != null) {
stream.close();
}
} catch (IOException e) {
//ignore
}
}
}Here the logging of any exceptions happens as normal and then in the finally we just want to make sure that the stream is closed. Unfortunately as the close method throws IOException, we have to have yet another try/catch when we don't care about the exception at all. FileUtils simply provides a tidier way of acheiveing this.
Is it a good thing to simply swallow exceptions? That's a whole other story and I can see both sides of the argument but I'll sit on the fence ;).
When there is more than one thing to be closed - e.g. an InputStream and an OutputStream - using nested try-finally blocks makes it clearer that everything gets closed, with no "if (os != null) {....}" weirdness:Finally...
try { InputStream is = new FileInputStream(...); try { OutputStream os = new FileOutputStream(...); try { copyStuff(is, os); } finally { os.close(); } } finally { is.close(); } } catch (IOException e) { throw new BuildException(e); }
Thoughts?
I can see where your'e going with this. You use the catch to wrap everything, and don't catch locally. The only problem is where you want to log the exact message at the time of the exception do you lose the ability when you catch at the end? Also I like the way you don't check for nulls - is this NullPointerException safe? if 'is' is null when you call close in finally (guaranteed to happen), you'll get a NullPointerException - which is a RuntimeException, so will propogate back to the user.
my thoughts...;) Kev
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
