Empty Catch Clause

You see it all over in Java examples and code: EmptyCatchClauses;

try {
someObject.something();
} catch(Exception e) {
// should never happen
}

Either code can handle the exception, then the catch clause shouldn't been empty, or the code can not handle the exception, then there should not be a try/catch block at all.

Oh, the one designing the interface of the something() method screwed up? Well, try to get it fixed in the method first. If this is not possible, just let the exception pass through (ThrowDontCatch). If this is not possible (why shouldn't it?), subclass the class of someObject, and provide a wrapper method, which contains the dreaded EmptyCatchClause OnceAndOnlyOnce.

---

Although it would be nice if every exception were valid, that's not always the case. Sometimes a piece of code throws a checked exception that you know can't occur. Perhaps it's unique to your situation, or you're wrapping a bit of poorly-designed code. In any case, you don't want to propagate the exception (because then someone else will have to spend cycles dealing with it) and there's no need to handle the exception. In these cases, I like to use an assertion. For example, in a single-threaded app, InterruptedException can't occur:

 public void sleep(long delayInMilliseconds) {
try {
Thread.sleep(delayInMillisecond);
}
catch (InterruptedException e) {
Assert.impossibleException(e);
}
 }

Overall, though, I agree. Empty catch clauses are a definite CodeSmell. -- JimLittle

What about this instead?

 catch ( InterruptedException iE ) {
// somebody interrupted the thread so I could tidy up and stop, maybe I should 
// have allowed the exception to propagate, or stop and reset the interrupted flag
// rather than asserting the impossible.
 }

-- AnonymousDonor

---

Contrast it to this:

try {
someObject.something();
} catch(Exception e) {
// don't care if it happens
}

This still has an empty catch clause, but it has handled the exception. This isn't used for "can't ever happen" scenarios; it's used for "can happen, but it doesn't matter if it does".

Note, it's been pointed out to me that expressing this as

try {
someObject.something();
} catch(Exception unimportant) {}

is more expressive code; it doesn't need the comment.

---

I would say definitely don't have an empty catch() for the base classes like Throwable or Exception. The someObject.something() call may call many other methods and if one of those throws an Exception, it will be silently caught and it is very frustrating trying to track it down. In these cases I catch an more specific exception (like InterruptedException) or at least add an e.printStackTrace() as a temporary measure. -- RobNielsen?

---

I say it is valid to have an empty catch clause: The unchangeable 3rd party package you're using throws an exception for a condition you expect to happen during the normal course of execution of your program, and they didn't think to provide a way to give you a return code instead.

For example, I've worked on several applications that would INSERT a record and watch for a "duplicate index entry" error to handle the 2% of the cases where there was a conflict. Now one would rather check for that particular error and handle it as a return code, so that alternate business logic can be executed, but I've yet to see a 3rd party database interface that uses exceptions and also gives you that level of control. (On one project I wrote such a package - in a layer of C++ wrapper classes over ODBC.) So, if you want the main line of your business logic to be rational, you'll want to catch the errant exception and turn it into a special return code. The catch block can still end up empty because you might also be using the defensive coding style of setting the return code to "fail" before the try block, and only setting it to "success" at the end of the successful operation. (IE: at the end of the try block.) -- JeffGrigg

Can somebody elaborate on "defensive coding", perhaps with an example?

Example:

  successFlag = FALSE;
  try {
if (doSomethingAndReturnResult() == resultWeLike)
successFlag = TRUE;
  } catch(ExceptionTypeWeExpect e) {
// 'successFlag' is already FALSE.
  }
  return successFlag;

---

I think it's the "Can't Ever Happen" comment that represents the smell here. If something "can't ever happen", about five minutes after it gets into production you'll have a long and exhausting debugging session when it does. In such a case, you should throw a runtime exception; after all, one of the fundamental assumptions in the system has just been proven wrong.

In the case where there's an empty catch block because you've recognized it can happen, and your way of handling it is to do nothing (as in Jeff's defensive coding technique), it's not a smell. The possibility has been recognized, and the exceptional condition has been dealt with correctly (by ignoring it). -- RobertWatkins

Ooh, ooh, I have one of these. Consider the case of some session management code that periodically expires sessions.

try
{
for (Iterator i = iterator(); i.hasNext();)
closeSession( (Session)i.next() );
}
catch (ConcurrentModificationException cme) {}

One could write some complex exception handling code to try again until all the expired sessions are cleaned up. But the expiration code will try again very soon. So, the empty catch IS handling the exception by explicitly saying that the right action is to ignore it. In which case, you should consider this to make that explicit:

try
{
for (Iterator i = iterator(); i.hasNext();)
closeSession( (Session)i.next() );
}
catch (ConcurrentModificationException ignored) {}

---

See below if you object to this code stopping before the iterator is empty. I'm still a bit puzzled whether a new iterator is created during every iteration, and if so why. If there is some thread issue, isn't there a better way to handle it?

---

I just love this one - we used to have a programmer working with us who did this so much, we now use his name to describe this "technique".

Another variation (which seems to be caused by programmers inserting print statements everywhere during frantic debugging sessions) is wrapping every line of code with

try {
... something ...
} catch (Exception e) {
e.printStackTrace (System.err);
throw e;
}

(sometimes nested several levels deep) so whenever you get an exception it's printed out five or ten times over and over. Joy. -- JamesWilson

That's just silly. -- rw

---

One solution I prefer is to use RuntimeException. It makes for cleaner code and bugs show up very prominently when you hit an uncaught exception. -- TimBurns

I'm disappointed that RuntimeException doesn't have a constructor taking an exception. It would be nice to promote a checked exception to an unchecked one for cases like these. Fortunately, there are a number of implementations of NestedRuntimeExceptions available. -- MarkAddleman

No big deal, just write your own WrappedRuntimeException that can hold the real exception inside. -- KyleCordes

Sun has recognized this as a flaw - in Merlin (Java 1.4), there will be a new method on the Exception class allowing you to chain them up. I don't know if they'll extend the constructors, but it does make sense -- rw

Yes, Sun did add "cause" arguments to some constructors.

---

Empty catch clauses are a definite CodeSmell.

I actually vacillate about this. OnceAndOnlyOnce drives me to want to use Java's checked exceptions as a poor man's extended message syntax.

Without using exceptions, I would write something like

 Row row = retrieveDbRow(key);
 if (row == null) {
do something
 }

Assuming that I need to retrieveDbRow and do something if key does not exist more than once, duplicating the if-statement violates OnceAndOnlyOnce. But, more importantly, it forces me to remember to check for null every time. By making NotFoundException? a checked exception, the compiler remembers for me.

 try {
Row row = retrieveDbRow(key);
 } catch (NotFoundException e) {
do something
 }

Now, the compiler is working for me and the control flow is pretty clear. It's not as clear as I'd like, but if there is only one statement that throws a NotFoundException and, by convention, that statement is at the start of the try block, the control is pretty clear.

The upshot of this style is that there will be cases with EmptyCatchClauses:

 try {
Row row = retrieveDbRow(key);
 } catch (NotFoundException e) {
 }

But, I think this is a more deliberate expression of programmer intent than not having the if-statement.

-- MarkAddleman

I think this is a misunderstanding of the concept of a CodeSmell. From CodeSmells: A perfectly good idiom may be considered a CodeSmell because it's often misused, or because there's a simpler alternative that works in most cases. Calling something a CodeSmell is not an attack; it's simply a sign that a closer look is warranted. -- AlexChurchill

---

With the pending release of Java 1.4, and the addition of an assert() facility built into the language (as opposed to those hacks which all of us have been using so long), I would say that the correct coding of the initial example is as follows:

try {
someObject.something();
} catch(Exception e) {
// should never happen
assert(false);
}

Here, the assert(false) demonstrates that we know this is an 'impossible' condition (are they ever?), but also tells the runtime to alert us if we were wrong, which just the comment cannot do.

Remember that assertions can be switched off and then we would never get this exception. It would be better to log so that program does not crash but the administrator/user/developer can check the logs for errors. -- VhIndukumar

If the code is not already tied to Java 1.4, it might be excessive to make it so just for the benefit of something that can never happen.

---

I would be careful about what assumptions you make about the code that threw the exception. In several of the examples above, the assumption is made that the code fully completed and just threw an exception. In other examples, it is assumed nothing has happened. The latter is closer to the intent behind exceptions, but I would take care not to rely upon it. -- WayneMack

---

try
{
for (Iterator i = iterator(); i.hasNext();)
closeSession( (Session)i.next() );
}
catch (ConcurrentModificationException cme) {}

Shouldn't this be more something like:

for (Iterator i = iterator(); i.hasNext();)
{
try
{
closeSession( (Session)i.next() );
}
catch (ConcurrentModificationException cme) {}
}

No, because this is an infinite loop

Maybe I misunderstand, but I'm not sure why one would want to stop closing ALL sessions because one happened to throw an expired exception. Of course, if this exception is normal and expected, I don't think I would rely on an exception to control program flow - instead, I would probably do something like this:

...
void closeSession( Session ctx )
{
if ( ! ctx.isExpired() )
ctx.close();
}
...

for (Iterator i = iterator(); i.hasNext();)
{
try
{
closeSession( (Session)i.next() );
}
catch ( Throwable e )
{
Logger.printException( e ); // print error and continue
}
}

This will also execute faster since it does not have to go into the catch block for normal conditions. Also, if there is some unexpected error, it will be logged so that techsupport or developers can see the unusual condition. -- RobertDiFalco

I guess I was not being very clear. The ConcurrentModificationException will only happen if someone is trying to add or delete a session while you are going through the list checking for expired sessions. Handling that case seems useless because another aging pass will be made shortly. Also, the iterator is invalid after the exception, so moving the try inside the loop would not work.

The first example was snipped too much. Here is the full method:

private void ageSessions()
{
try
{
for (Iterator i = iterator(); i.hasNext();)
{
Session session = (Session)i.next();
if (!session.isActive())
session.close();
}
}
catch (ConcurrentModificationException cme)
{
The iterator may bail out if someone tries to add or delete
a session while we are iterating (fail-fast). When this
happens, we don't need to do anything. If we missed any
sessions, they will get expired next time around.
}
}

--MichaelBanks

Hmmm... in that case, have you considered using an internal iterator and synchronizing on that so that a ConcurrentModificationException? is not possible?

sessions.enum( new SessionBlock() {
public void run( Session each ) {
if ( each.isActive() )
each.close(); } } );

This makes the transaction across the list atomic and makes the contract for its interface simpler. -- RobertDiFalco

I just read the excellent explanation of InternalIterator and BlocksInJava. This is a very nice way to avoid the empty catch clause. The only remaining concern is that synchronizing the iterator holds off any opens or closes of new sessions for the duration of the iteration. -- MichaelBanks

I'd think the lock is exactly the problem, at least from one point of view. The first example would be low-priority, pre-emptible cleanup. The locking one might still be low-priority, but it's pre-emptive. Which one you'd choose would depend on whether accumulating garbage has enough consequences that you'd be willing to compromise the main path in order to clean it up. In the case of session management, I'd probably choose the former, but there might be resource leak problem as you scale (cleanup never happens because sessions come in faster than you can expire them), so you might need to move to the second or a hybrid model. --ChrisMellon?

---

What about the problem where closing a stream can throw an IOException.

  FileInputStream fis;
  try {
fis = new FileInputStream("foo.txt");
//  do stuff
  } catch (IOException e) {
// warn user
  } finally {
if (fis != null) {
try {
fis.close();
} catch (IOException e) {}  // EmptyCatchClause?
}
  }

I'm sure the XP theorists really hate that style of code. But, what can you tell a user that makes any sense? At least the common practice in C++ classes is that destructors are written to never throw exceptions.

Well, you could create an InputSteamAdapter that hides the try...catch around the #close call. I still wouldn't keep it empty. I would probably write a log entry just in case. -- RobertDiFalco

I'd write a log entry because I want to know under what circumstances closing a file can fail...

---

Well, I prefer to explicitly throw an exception:

try {
someObject.something();
} catch(Exception canNeverHappen) {
throw new ImpossibleRuntimeException(canNeverHappen);
}

The advantage over the assert style is that the compiler knows an exception will be thrown and won't force me to put in unnecessary return statements etc.

-- NeilSwingler

I like this a lot because I have a strong feeling that, as something() changes, these will start popping up for reasons we couldn't have imagined at first writing. -- DustinAleksiuk

I HaveThisPattern -- PhilippeDetournay

---

It is even worse in C++:

try {
someObject.something();
} 
catch ( ... ) {}

In case something fails, the code will do nothing and continue happily processing. I've seen several times this wrongdoing for avoiding crashes, since this catch even catches access violations (on some platforms, which could reasonably be described as broken for this reason among others). On sane systems, a catch (...) will not interfere with handling of access violations or similar. This is not a C++ problem; it's an MSVC++ problem.

---

Note that PeeEmDee can find this problem.

---

Whenever its been said "it can never happen", I have suggested putting a System.exit(1) - people aren't so certain then ;) -- ChanningWalton

---

I use something like this for unit testing exception throwing (with JavaUnit).

 try {
a.mustThrowMyException();
fail("My exception should be thrown");
 } catch (MyException e) {
// This SHOULD happen
 }

I feel like it's a good use of an empty catch clause.

Thank you for coming up with the only sane example of an empty Java catch block on this page. Sorry folks, if you catch one of these other examples you could at least log it. What? You say you don't have one... 'nother smell I suppose.

Eh, how about:

 try {
a.mustThrowMyException();
fail("My exception should be thrown");
 } catch (MyException expected) {
 }

---

Sorry but I'm going to have to vote for 'Emty catches are sometimes OK'. Take this for example:

// (try to) set native L&F
try {
UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());

// don't bother handling these, we'll just end up with the
// cross-platform look and feel which is fine.
} catch (ClassNotFoundException e) {
} catch (InstantiationException? e) {
} catch (IllegalAccessException? e) {
} catch (UnsupportedLookAndFeelException? e) {
}

If anything goes wrong, I'm perfectly happy to accept the default look and feel. What would others suggest I do in this situation? Would adding log statements suffice to make the catches non-empty?

Here's another one:

try {
System.setProperty("java.net.useSystemProxies", "true");
} catch (SecurityException? e) {
// ignore
}

I would ideally like to use the system proxy, but if this method fails for whatever reason, I'm happy to continue. So both of these code examples involve trying to set some property or feature, but where failure is acceptable because of sane defaults. MikeWeller.

---

And what about this?

Socket s = new Socket("somehost",somePort);
socket.setSoTimeout(100);
try {
ObjectInputStream? ois = new ObjectInputStream?(s.getInputStream()); // it will wait until any data arrive, (or for 100ms, then throw an exception)
}
catch(SocketTimeoutException? e) { /* This is an expected exception, it's a normal working */ }
// and i don't wanna wait forever, that's why i'm using timeout.

// and later:

Object o;
RUN = true;
while(RUN) {
try {
o = readObject(ois);
process(o);
}
catch(SocketTimeoutException? e) { /* This is an expected exception, it's a normal working */ }
doSomeOtherStuff();
}

Let's say, i wanna check RUN in every 100ms, and exit if it's false. (this is a thread, other thread can set RUN=false) MartonSuranyi.

---

CategoryCodeSmell. See also ReplaceEmptyCatchWithTest.