Empty Catch Blocks are Always Wrong

I like to read code. It has some terrible consequences, though. It seems that every time I get comfortable with a third-party API (or even java standard APIs) and I got to read the source, I find some terrible bugs.

I realize this sounds exactly like java exception anti-patterns. I didn't really start out with that goal, but today I was using an API where one class used another seemingly unrelated class and I wanted to know what it did with it. What I found was astonishingly bad error handling throughout the code base.

Nearly every single thing that could possibly throw an exception was wrapped in a try block with an empty catch block.

This is security code. You may very well rely on this code as part of your infrastructure.

I was going over this with a friend who said, It's hard to separate what your writing from what you're doing. I agree with this point. I'm a huge advocate of laziness in software development and think people should really spend more of their time perfecting laziness.

For example, when you're writing some code involving the filesystem or network and you call a method that might throw an IOException that you don't want to deal with now, which is the better option for the lazy programmer?

1. Declare the exception (or java.lang.Exception) in your method signature.
2. Wrap the code in a try block and save it for later.

If absolutely everything goes your way every time, it probably doesn't matter much. However, the moment something isn't working correctly, you've got to work to figure out why. If you chose option #1, figuring out why is mostly done for you because you'll end up with a stack trace that points you to the line of code that failed. If you chose option #2, you probably consume more caffeine than I do.

So the code I'm looking at is full of #2. You might call it #2-like, or perhaps, shitty. If I invoke a method that very specifically exists only to perform an IO operation, it provides no means by which I may know whether it succeeded or failed. Let's examine two slightly scrubbed methods to see how this goes:

  public void write(String filename)
    throws FileNotFoundException, IOException;
  public void write(OutputStream os);

In that example, if I want to write to an OutputStream, I'm in bad shape. I can't possibly know whether it worked or not. However, if I want to write to a file, I seem to get plenty of useful feedback, right? Well, no, write(String) is implemented by calling write(OutputStream). I can tell if it can't make a file, but I can't tell if it can't write to it. That's because write(OutputStream) looks like this:

  try {
    // to write stuff out
  } catch(Exception e) {
  }

There's simply no way to look at that and see it as anything other than a bug someone took the effort to type up. I can't imagine how many bugs exist just because of that extra stuff around the import stuff in the middle.

I'm going to go ahead and declare the following categorically wrong. If you write something like this, you just took the time to write a bug and you can't be my friend anymore:

1. Empty catch blocks.
2. System.out.whatever or System.err.whatever within a catch block.
3. printStackTrace() in a catch block.

For my java projects, any empty block is considered a warning (and I fix all warnings). For my main work project, buildbot will fail to compile your code if it says System.out, System.err, or printStackTrace anywhere in it. If you need to log stuff, use a logger. If you caught an exception, you've got to have a really good reason and a really good plan on what you intend to do with it.

If you don't or are otherwise lazy, just don't catch it. Let it go up. Let the user see it. Really. A user seeing an error is much better than your application just lying to them about what it's doing with their data.