7

Example #1: 

try { fileChooser.setSelectedFile(new File(filename)); }
catch (NullPointerException e) { /* do nothing */ }

Example #2: 

if (filename != null)
    fileChooser.setSelectedFile(new File(filename));

Is #1 inherently bad, for performance or stability (or any other reasons), or is it just a little different? This isn't a very good example because there are no advantages to #1 over #2, but under different circumstances there could be (e.g. improved readability, fewer lines of code, etc.).

Edit: The consensus seems to be that #1 is a no-no. Most popular reason: Overhead

Also, @Raph Levien had a good insight:

One reason to avoid #1 is that it poisons your ability to use exception breakpoints. Normal-functioning code will never purposefully trigger a null pointer exception. Thus, it makes sense to set your debugger to stop every time one happens. In the case of #1, you'll probably get such exceptions routinely. There's also a potential performance impact, among other good reasons.

Also, See Link for more depth.

Community
  • 1
  • 1
TheBlackKeys
  • 852
  • 1
  • 9
  • 15
  • I would think the "clean code" book would favor option 2 over option 1. Try /catches are usually good for trying to group "operations" together that you can undo or reverse if you get an error. If you take the example of sockets, you could check to see the socket is open, but find that you can't write to it anyways down the line. So you really need to use a mix. – CtrlDot Mar 26 '11 at 03:08
  • 1
    the main issue w/ the 1st example is that there is no guarantee where your NPE occurs, if may or may not be the faulty filename, thus you might prefer not to catch. Exception in java have very little overhead, so do not avoid them based solely on superstitions that they are slow. – bestsss Mar 28 '11 at 23:32
  • Overhead is not the reason not to use #1. When you throw exceptions, it is for them **not to be caught except at top level** (or occasionnaly). If you are cluttering your code with try catch blocks, you're better using normal logic (ie. #2). If the fact that the file does not exist warrants termination of program (ie. a config file, a key file, etc), you should let the exception bubble up. – Alexandre C. Apr 01 '11 at 22:06

7 Answers7

8

Definitely #2 is a better choice. Exception handling should not be used as a construct in the control flow of your program. Exceptions should be used to handle situations that are not under the control of the programmer. In this example, you can check if the fileChooser is null so that is under your control.

Vincent Ramdhanie
  • 98,815
  • 22
  • 134
  • 183
6

Exceptions do bring a certain amount of overhead with them. If they can be avoided, in this case by simply checking for a null, that would be preferred.

As has been said by others, it is also generally bad practice to use Exceptions for flow control. For a detailed explanation of why this is bad, check out this.

Here is a short blurb from that answer:

Exceptions are basically non-local goto statements with all the consequences of the latter. Using exceptions for flow control violates a principle of least astonishment, make programs hard to read (remember that programs are written for programmers first).

Community
  • 1
  • 1
Corey Sunwold
  • 9,892
  • 5
  • 47
  • 55
5
  • Don't use Exception to control logic flow.
  • Never swallow Exception. #1 violates it- you'll never know if NPE is from fileChooser==null or fileName==null.
卢声远 Shengyuan Lu
  • 29,208
  • 21
  • 78
  • 123
4

If a null value is logically allowed by your program, test for it. Otherwise use exceptions. Exceptions are meant to deal with exceptional conditions (surprise!) and shouldn't be used for program logic.

CromTheDestroyer
  • 3,366
  • 3
  • 17
  • 26
3

One reason to avoid #1 is that it poisons your ability to use exception breakpoints. Normal-functioning code will never purposefully trigger a null pointer exception. Thus, it makes sense to set your debugger to stop every time one happens. In the case of #1, you'll probably get such exceptions routinely. There's also a potential performance impact, among other good reasons.

Raph Levien
  • 4,898
  • 23
  • 23
2

I'd go for option #3 - if filename is passed as a function argument, put a note in the comments that a null filename is not allowed. Then just work with the assumption that filename is never null:

fileChooser.setSelectedFile(new File(filename));

If some idiot (or you, <3) ignores your comment and ''does'' pass a null, he'll get a NullPointerException and has to fix the problem.

In the client code, ensure that no nulls are passed by always initializing or defaulting the fileName to an empty string.

I've been avoiding using and accepting nulls for a while now, and my code is much cleaner now that there aren't as much null checks in it anymore. The only null checks still present are those where I call third-party code that may return null.

(rant: Of course, I have to decompile said third-party code and determine that myself, considering the documentation is either not present or doesn't make note of it possibly returning null)

cthulhu
  • 3,075
  • 2
  • 20
  • 31
2

Your two examples are not semantically equivalent in the following cases:

  • fileChooser is null
  • setSelectedFile throws a NullPointerException (granted, this can't happen with this particular method, but in general it's quite possible for a setter to store stuff in some other object, which will throw if the reference for that other object was not initialized)
  • new File() throws a NullPointerException for reasons other than filename being null

Probably, the /* do nothing */ was intended only for the case where filename is null, but now hides the other cases too, whereas good exception handling would allow these exception to bubble up the call stack, alerting you to the presence and cause of a bug you would otherwise find difficult to track down.

I find this the by far more compelling reason than performance, because wasted programmer time is a lot more expensive than wasted cpu time.

meriton
  • 61,876
  • 13
  • 96
  • 163