-1

Bear with me, as I've read a bit on this and I'm still not sure if I'm thinking about this correctly, but I want to understand this fully...

I've recently found myself in a debate that surrounds a pragmatic approach to code, whereby the feedback I've been given is (I think) a misrepresentation of general Software Engineering advice.

So consider you have a method that handles an input - let's say you take in a String and format in a particular way (assume it's not a way that any existing Java until provides). The method could throw a NPE if null is passed in, because that shouldn't happen.

I didn't add in any NPE handling like the Apache Commons library for StringUtils has, where when you pass in null it provides back an empty string because, in the context of the method, a null shouldn't be provided. I wanted to leave it up to the implementation to determine how to handle this.

The logic for this, by me, is that you wouldn't necessarily want this null value to just be swallowed as normal, but might want to catch this and rethrow the occurance as another exception to explain "this shouldn't have happened" - say, if it was for a step that was considered mandatory.

A colleagues said that "catching NPEs is considered bad practice", but further research on this suggests this is only the case where you're ignoring the NPE (or swallowing it).

To my mind, that's what 

if(var = null) { return ""; }

(for example) is doing, whilst actually handling the null as an exception...

try {
    myStringMethod( somevar );
}
catch ( final NullPointerException e )
{
    Throw new MySpecificException( e );
}

... Is an effective way of handling a mandatory value that shouldn't be null.

In the way it was put across, it seems to suggest a NPE should never be seen - which I agree with, but surely "dealing with it" by somewhat ignoring the cause is actually the wrong way?

Chris J
  • 1,441
  • 9
  • 19
  • 1
    *"to explain "this shouldn't have happened""* The NPE itself already says that, by the simple nature of having been thrown. What you *could* add to it, is what you were doing when it occurred, e.g. "Parse Exception while parsing line 666: NullPointerException", but you might want to do that to all exceptions, not just NPE, so explicitly catching NPE is not really what you want, you want to catch `RuntimeException`, or `Exception`, to cover them all. – Andreas Jul 22 '19 at 20:38
  • *FYI:* `if(var = null)` won't compile, try `if (var == null)` instead. – Andreas Jul 22 '19 at 20:40
  • @ButiriDan - it's not a duplicate at all - the question is the concept of 'catching', which that question does not answer at all in any context. – Chris J Jul 22 '19 at 20:40
  • 1
    In general, you want methods to be permissive in what inputs they can handle without breaking, and restrictive in what outputs they return. This usually means gracefully handling null inputs, and wrapping potentially-null outputs in an Optional to force the caller to handle the null case. You should avoid relying on catching a NullPointerException just because you don't want to write a null check. If a null input is invalid, then by all means test for null and then throw something like an IllegalArgumentException. – Jordan Jul 22 '19 at 20:41
  • @Andreas yes, it was just purely for example as written on my phone, not actual code. Dealing with the NPE by rethrowing with context is what the 'catch' in the example would do - e.g. rethrowing a specific that would translate to 'value supplied should not be null' or similar, or in a more specific case, it would throw a specific exception for a web service to say that a specific value was not provided that should have been. – Chris J Jul 22 '19 at 20:43
  • If the input should never be null then there is negative value in checking, as this enables bad upstream code. – Andrew S Jul 22 '19 at 20:43
  • @ChrisJ NPE already says "'value supplied should not be null" and the stacktrace already tells you which method detected the problem. My point was that simply re-throwing with a different exception doesn't add anything you don't already know. You need to rethrow with a wrapping exception that *adds* dynamic information, e.g. line number, id or name of object being processed, or some other dynamic information that'll allow whoever reads the error to better **research** why the error occurred, so it can be fixed. – Andreas Jul 22 '19 at 20:48
  • @Andreas - I don't understand how that is any different than what has been stated - you catch the NPE and rethrow something more specific. I've provided a contextual, rather than actual/specific example. The question is, why is simply absorbing the null to *avoid* a NPE considered a better practice than preventing the continuation of an undesigned/unexpected scenario? – Chris J Jul 22 '19 at 20:51
  • *"it would throw a specific exception for a web service to say that a specific value was not provided that should have been"* That should have been explicit validation done up-front by the web service logic, where the exception would know exactly how to phrase the error message to be meaningful, in which case an NPE would never be thrown. Which is the main reason for the guideline of "don't catch NPE", i.e. NPE indicates that the data wasn't correct to begin with and should have been handled before it even got this far. – Andreas Jul 22 '19 at 20:51
  • *"why is simply absorbing the null to avoid a NPE considered a better practice"* It is not. The "don't catch NPE" doesn't mean that you should absorb it. It means that some up-stream code didn't do what it was supposed to, and to go fix that, meaning that the NPE correctly indicates an error elsewhere, so catching it generally means you're trying to fix the *symptom*. not the *cause*. Catch and re-throw with more information, particularly dynamic (data) information, is valid, but simply explaining the null with other words doesn't add anything useful. – Andreas Jul 22 '19 at 20:54
  • The only time I would throw an NPE is when it is likely a NULL input really, really means an upstream failure. This means your method call needs to understand why it might be called with a NULL and determines that should be a failure condition. And NO, you shouldn't trap NPEs but allow them to be reported and program termination. Just my opinion. – Joseph Larson Jul 22 '19 at 20:56
  • In general, it's bad practice to catch a `RuntimeException` – Blake Jul 23 '19 at 02:00

1 Answers1

2

NullPointerException is (almost) always a bug. Either a method accept null as an argument, in which case it should never throw NPE. Or it doesn't accept null, in which case the caller is responsible for not passing it.

Catching NullPointerException is (almost) never the solution.

Lars Christian Jensen
  • 1,407
  • 1
  • 13
  • 14
  • 1
    *"Or it doesn't accept null, in which case the caller is responsible for not passing it."* Does this mean that the method should still throw an NPE, if it "doesn't accept it"? – Chris J Jul 22 '19 at 21:13
  • Personally, I'd say yes, the method should throw an exception to let the caller know that they've violated a contract. – NPras Jul 23 '19 at 00:57
  • A method will usually throw an NPE if null is passed where it shouldn't. Or for any other bug that causes NPE, which could be unrelated to the parameters. By catching `NullPointerException` you may mask those other bugs unintentionally. – Lars Christian Jensen Jul 23 '19 at 06:18