35

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,352
  • 5
  • 75
  • 130
  • 4
    For a real challenge, write a unit test to make sure this catch actually works. – Jay Elston Nov 29 '11 at 16:35
  • 1
    `throw new ImpossibleException("Reboot the universe. Things are messed up", e); – Chris Cudmore Nov 29 '11 at 17:46
  • 1
    "Won't ever happen" will... –  Nov 30 '11 at 10:29
  • Thorbjørn Ravn Andersen: it might after changing the program, but at least in its current state, no set of input data can trigger the exception. – user281377 Nov 30 '11 at 12:45
  • In your specific example above, an exception is thrown because the method does not know whether it will be called with a supported or not supported encoding. A way around this would be if there was another constructor that assumed a standard charset was given, which wouldn't need to declare that an exception would be thrown. – jordan May 21 '14 at 22:51

5 Answers5

35

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
  • 4
Fredrik
  • 803
  • 6
  • 12
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,534
  • 9
  • 64
  • 93
  • 4
    The real bad decission (or omission if you want) is that there are no pre-defined Charset instances for those 6 charsets that Java requires. `foobar = new InputStreamReader(p.getInputStream(), Charset.ISO_8859_1);` - now wouldn't that be nicer and avoid any mistake once and forever? – user281377 Nov 29 '11 at 23:01
  • 3
    The Guava library has tons of these things. Makes life easier. –  Nov 30 '11 at 10:30
  • 3
    Java 1.7+ does have charset constants defined: http://docs.oracle.com/javase/7/docs/api/java/nio/charset/StandardCharsets.html. Use them like so: StandardCharsets.UTF_8.displayName() – Michael Oct 03 '14 at 16:19
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,427
  • 16
  • 160
  • 185
  • Good idea; a simple `assert false;` doesn't add too much clutter and makes it clear that I assume the catch block will never be entered. – user281377 Nov 29 '11 at 09:35
  • 5
    @ammoQ, or you can even add a message to make your intent absolutely clear: `assert false : "should never happen"`. – Péter Török Nov 29 '11 at 09:37
  • 13
    Even better, "Should never happen because Java requires the charset ISO-8859-1 to be supported." – dan04 Nov 29 '11 at 15:49
  • 4
    `assert` implies assertions are enabled. I throw an `UnexpectedException` (which also has the benefit of letting me have a stack trace...). – Chop Nov 04 '15 at 09:15
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,462
  • 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,841
  • 1
  • 24
  • 47