Is this code defensive programming, or bad practice?

Question

I have this debate with my colleague about this piece of code:

var y = null;
if (x.parent != null)
    y = x.parent.somefield;

My point of view is that in the place where the code is, x.parent should not POSSIBLY be null. And when it is null, we have a serious problem and I want to know it! Hence, the null check should not be there and let the down stream exception happen.

My colleague says this is defensive programming. And the null check makes sure the code doesn't break the application.

My question is, is this defensive programming? Or a bad practice?

Note: the point is not who is right. I'm trying to learn from this example.

Answer 1

Interesting question. From my point of view, whether or not to include the check is a matter of how well the data is validated, where does it come from and what can happen when the check fails.

"x.parent should not POSSIBLY be null" is a serious assumption. You need to be very sure of it to play safe, of course there is the classic "it will never happen"....... until it happens, that's why I think it's interesting to review the possibilities.

I see two different things to think about.

Where does the data comes from?

• If it comes from another method in the same class, or from some related class, since you have more or less full control about it, it's logical to relax your defenses, as you can reasonably assume that it's very unlikely to have bad data to begin with, or if it happens it's reasonably easy to catch the bug early during debugging/testing and even place some unit tests for it.

• The opposite case would be if it's data entered by the user, or read from a file, or from a URL, anything external generally speaking. Since you cannot control what the program is dealing with: by all means, validate it as thoroughly as you can before using it in any way, as you're exposed to bogus/missing/incomplete/incorrect/malicious information that may cause problems down the path.

• An intermediate case can be when the input comes from another layer/tier within the same system. It's more difficult to decide whether to do a full validation or take it for granted, since it's another piece of internal software, but might be replaced or modified independently at a later time. My preference goes towards validating again when crossing boundaries.

What to do with the validation?

• Using an if (as in your sample) to simply skip over some assignment or usage may be fine for some cases. For instance, if the user is entering some data and this just shows a tooltip or other minor info, it's possibly safe to skip. But if the piece of code does something important, that in turn fills some mandatory condition or executes some other process, it's not the right approach as it will cause problems for the next code run. The thing is, when you skip over some code, it must be safe to do so, without any side effects or unwanted consequences, otherwise you would be hiding some error, and that's quite difficult to debug in later development stages.

• Abort the current process gracefully is a good choice for early validations, when the failure is totally expected and you know precisely how to respond to it. An example could be a missing required field, the process gets interrupted and a message is shown to the user asking for the missing info. It's not OK to simply ignore the error, but also not serious enough to throw an exception that disrupts the normal program flow. Of course, you may still use an exception, depending on your architecture, but in any case catch it and recover gracefully.

• Throw an exception is always a choice when the "impossible" really happened. It's in those cases where you cannot possibly provide a reasonable response for either continuing with some variation or cancel just the current process, it may be due to a bug or bad input somewhere, but the important thing is that you want to know it and have all the details about it, so the best possible way is to make it to blow up as loudly as possible, so that the exception bubbles up and reaches a global handler that interrupts everything, saves to a log file/DB/whatever, sends a crash report to you and finds a way to resume execution, if that's feasible or safe. At least if your app crashes, do so in the most graceful way, and leave traces for further analysis.

As always, it depends on the situation. But just using an if to avoid coding an exception handler is for sure a bad practice. It must always be there, and then some code may rely on it - whether appropriate or not - if it's not critical to fail.