Test For Null In Exception Catch Block Indicative Of Anti-Pattern
In a blog posting about Project Coin I came across this example: (first comment is mine)
// Please do not write code like this:
private static void customBufferStreamCopy(File source, File target) {
InputStream fis = null;
OutputStream fos = null;
try {
fis = new FileInputStream(source);
fos = new FileOutputStream(target);
byte[] buf = new byte[8192];
int i;
while ((i = fis.read(buf)) != -1) {
fos.write(buf, 0, i);
}
}
catch (Exception e) {
e.printStackTrace();
} finally {
close(fis);
close(fos);
}
}
private static void close(Closeable closable) {
if (closable != null) {
try {
closable.close();
} catch (IOException e) {
e.printStackTrace();
}
}
}
At first sight and leaving the typos (closable) and other naming issues for what they are, this looks okay. There are however several problems with this code:
- The code is monolithic.
- Callers of
customBufferStreamCopyhave no idea if the call failed or succeeded.
Since this method is a simple API for copying files, callers should be notified if the copy action failed: the method should throwIOException. - Do Not Catch
java.lang.Exception. Ever. Please just don’t.
Never catchExceptiondirectly; catch one of its subclasses. And yes, there are exceptions to this rule, but if you do not know them you should simply adhere to the mantra: Do Not Catchjava.lang.Exception. - Variable initialization to
null.
This is an ugly hack to prevent compiler errors. Ugly hacks like this are indicative you are doing something suboptimal or even wrong. - Test for null in catch block.
Although I have seen this anti-pattern so many times, I still feel a little sad when I come across an instance of it. If you need to test wether or not something needs cleaning up, your exception handling is needlessly complex. The right way of using objects that need special cleanup or disposal is NOT:// the wrong way: needlessly complex try { // get reference to object // use object } catch (/* ... */ { // error handling } finally { // dispose of object only if object exists }It can be simpler:
// simpler is better (in this case at least) // get reference to object try { // use object } catch (/* ... */ { // error handling } finally { // dispose of object }
Incorporating the above criticisms, we can rewrite the copy method as follows:
// This is better…
private static void customCopy(File source, File target) throws IOException {
InputStream fis = new FileInputStream(source);
try {
OutputStream fos = new FileOutputStream(target);
try {
byte[] buf = new byte[8192];
int i;
while ((i = fis.read(buf)) != -1) {
fos.write(buf, 0, i);
}
} finally {
fos.close();
}
} finally {
fis.close();
}
}
Advantages:
- No if-statements: simpler execution flow.
- Better modularity.
- No compiler hacks.
- No need for a
closeutility method.
This code still looks monolithic, but as a direct result of the first two advantages it is now trivial to refactor the method into three separate methods:
// And this is best!
private static void customCopy(File source, File target) throws IOException {
InputStream fis = new FileInputStream(source);
try {
customCopy(fis, target);
} finally {
fis.close();
}
}
private static void customCopy(InputStream source, File target) throws IOException {
OutputStream fos = new FileOutputStream(target);
try {
customCopy(source, fos);
} finally {
fos.close();
}
}
private static void customCopy(InputStream source, OutputStream target) throws IOException {
byte[] buf = new byte[8192];
int i;
while ((i = source.read(buf)) != -1) {
fos.write(buf, 0, i);
}
}
Now we have three lean methods that are easy to understand and test. A lot better than the monolithic example we started out with.
Recent Comments