36

Example:

foobar = new InputStreamReader(p.getInputStream(), "ISO-8859-1");

Since the encoding is hardcoded and correct, the constructor will never throw the UnsupportedEncodingException declared in the specification (unless the java implementation is broken, in which case I'm lost anyway). Anyway, Java forces me to deal with that exception anyway.

Currently, it looks like that

try {
    foobar = new InputStreamReader(p.getInputStream(), "ISO-8859-1");
}
catch(UnsupportedEncodingException e) { /* won't ever happen */ }

Any ideas how to make it better?

user281377
  • 28,434

5 Answers5

36

If I had been given a cent for every time I've seen an log/error, "This should never happen", I would have... well, two cents. But still...

Empty catch blocks makes my spider senses tingle and most good code analyzer tools complain. I would avoid at all costs to leave them empty. Sure, now you know the error can never happen, but a year from now someone does a global search-replace of "ISO-8859-1" and suddenly you may have an extremely hard to find bug.

The assert false suggestion is good, but since assertions can be disabled at runtime, they are no guarantee. I'd use a RuntimeException instead. Those won't have to be caught by calling classes and if they ever occur you will have a stack trace to give full information.

Tripp Kinetics
  • 103
Fredrik
  • 813
28

I've always done it like this:

try {
    foobar = new InputStreamReader(p.getInputStream(), "ISO-8859-1");
} catch(UnsupportedEncodingException e) {
    throw new AssertionError(e);
}

May be a bit verbose (Java is...), but at least you'll get an assertion error when the impossible happens.

If the Java implementation is broken, you'll want to get as good error message as possible, as quickly as possible, instead of just ignoring the impossible. And even if the Java implementation isn't broken, someone could have changed your code to "UTF8" (oops - should it have been "UTF-8"?).

This should have been a runtime exception in the first place. JDK is full of this sort of wrong choices.

Joonas Pulakka
  • 23,562
27

My habit is, just to be on the safe side, to put an assert into the catch block. Someone might change the contents of the try block later, and you do want to know if the code fails don't you?

Péter Török
  • 46,547
4

If you are the only developer who will ever get to see this code then I'd say its fine, but if you are not then I would treat it as a real possibility or at least change the "won't ever happen" comment to something more useful.

Thanos Papathanasiou
  • 2,482
  • 1
  • 20
  • 22
4

The part of these exceptions that annoys me most is that it hurts my code coverage.

When I'm getting compulsive about coverage, I'll roll up the try / catch that "can never happen" (...or only if I'm using a mutant JVM that somehow forgot to include "US-ASCII") into a class and method that encapsulates that try / catch, and replace the checked exception in one of the ways mentioned here (usually throwing an unchecked exception with a snide message).

Then my code coverage takes a hit in the utility class, but not in all the references to that operation scattered around my code.

Sometimes I'll take the time to roll up like operations in a class that actually has coherent semantics. But since it's pretty obvious to my teammates what's going on, I usually just keep it as simple as I can & not worry so much about the best possible design.

However, as a comment mentioned, Guava & other libraries have ways to mitigate this pain -- but that's basically the same strategy. Move the annoyance off-stage so your main code doesn't take the hit in coverage.

sea-rob
  • 6,911