Is this code defensive programming, or bad practice? [closed]

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.

Preferential

throw an Exception when x.parent is null and handle it! – Betsey

best you change your question to a less opinion based before it gets removed. – Messapian

@Betsey I wanted to do that but I was told it creates exception handling code everywhere...... I still think it's better than hiding the problem. – Preferential

@Allen, tell your colleague, "welcome to real programming" ;) Naturally you need exception handling code everywhere, exception handling is a cross-cutting concern, like logging or data validation – Crusade

In case you choose to go with defensive method I would recommend logging to log file the cases you entered the defensive code condition. (and if needed throttle these loggings). It would always be good to know your code is not really behaving as expected! so log it to log file (with throttling perhaps to avoid log explosion). A metric counter for how many times you entered this code block could also be useful. – Deckhouse

Consider to migrate to codereview.stackexchange.com – Cacodemon

@MichaelFreidgeim This question would be off-topic on Code Review as it is missing context. In the future please link to the help center and use wording that shows the post may be off-topic when recommending Code Review. Take, "This may be on-topic on Code Review. Please check if it is on-topic and how to post a good question before posting there." – Gaudette

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.

Ringside

Where does the data comes from case number 3. This is answer is quite useful. – Preferential

Answer 2

It looks like your colleague is misunderstanding "defensive programming" and/or exceptions.

Defensive Programming

Defensive programming is about protecting against certain kinds of errors.

In this case x.parent == null is an error because your method needs to use x.parent.SomeField. And if parent were null, then the value of SomeField would clearly be invalid. Any calculation with or task performed using an invalid value would yield erroneous and unpredictable results.

So you need to protect against this possibility. A very good way to protect is by throwing a NullPointerException if you discover that x.parent == null. The exception will stop you from getting an invalid value from SomeField. It will stop you doing any calculations or performing any tasks with the invalid value. And it will abort all current work up until the the error is suitably resolved.

Note the exception is not the error; an invalid value in parent that is the actual error. The exception is in fact a protection mechanism. Exceptions are a defensive programming technique, they're not something to be avoided.

Since C# already throws an exception, you don't actually have to do anything. In fact it turns out that your colleague's effort "in the name of defensive programming", is actually undoing built-in defensive programming provided by the language.

Exceptions

I've noticed many programmers are unduly paranoid about exceptions. An exception itself isn't an error, it's simply reporting the error.

Your colleague says: "the null check makes sure the code doesn't break the application". This suggests he believes that exceptions break applications. They generally don't "break" an entire application.

Exceptions could break an application if poor exception handling puts the application in an inconsistent state. (But this is even more likely if errors are hidden.) They can also break an application if an exception 'escapes' a thread. (Escaping the main thread obviously implies your program has terminated rather ungracefully. But even escaping a child thread is bad enough that the best option for an operating system is to GPF the application.)

Exceptions will however break (abort) the current operation. And this is something they must do. Because if you code a method called DoSomething which calls DoStep1; an error in DoStep1 means that DoSomething cannot do its job properly. There is no point in going on to call DoStep2.

However, if at some point you can fully resolve a particular error, then by all means: do so. But note the emphasis on "fully resolve"; this doesn't mean just hide the error. Also, just logging the error is usually insufficient to resolve it. It means getting to the point where: if another method calls your method and uses it correctly, the 'resolved error' will not negatively affect the caller's ability to do its job properly. (No matter what that caller may be.)

Perhaps the best example of fully resolving an error is in the main processing loop of an application. Its job is to: wait for a message in the queue, pull the next message off the queue and call appropriate code to process the message. If an exception was raised and not resolved before getting back to the main message loop, it needs to be resolved. Otherwise the exception will escape the main thread and the application will be terminated.

Many languages provide a default exception handler (with a mechanism for the programmer to override/intercept it) in their standard framework. The default handler will usually just show an error message to the user and then swallow the exception.

Why? Because provided you haven't implemented poor exception handling, your program will be in a consistent state. The current message was aborted, and the next message can be processed as if nothing is wrong. You can of course override this handler to:

• Write to log file.
• Send call-stack information for troubleshooting.
• Ignore certain classes of exception. (E.g. Abort could imply you don't even need to tell the user, perhaps because you previously displayed a message.)
• etc.

Exception Handling

If you can resolve the error without an exception first being raised, then it is cleaner to do so. However, sometimes the error cannot be resolved where it first appears, or cannot be detected in advance. In these cases an exception should be raised/thrown to report the error, and you resolve it by implementing an exception handler (catch block in C#).

NOTE: Exception handlers server two distinct purposes: First they provide you a place to perform clean-up (or rollback code) specifically because there was an error/exception. Second, they provide a place to resolve an error and to swallow the exception. NB: It's very important in the former case that the exception be re-raised/thrown because it has not been resolved.

In a comment about throwing an exception and handling it, you said: "I wanted to do that but I was told it creates exception handling code everywhere."

This is another misconception. As per the previous side-note you only need exception handling where:

• You can resolve an error, in which case you're done.
• Or where you need to implement rollback code.

The concern may be due to flawed cause-and-effect analysis. You don't need rollback code just because you're throwing exceptions. There are many other reasons that exceptions can be thrown. Rollback code is required because the method needs to perform clean-up if an error occurs. In other words the exception handling code would be required in any case. This suggests the best defence against excessive exception handling is to design in such a way that the need for clean-up on errors is reduced.

So don't "not throw exceptions" to avoid excessive exception handling. I agree excessive exception handling is bad (see design consideration above). But it is far worse to not rollback when you should because you didn't even know there was an error.

Ludewig

@Gabriel While your attempt to improve the answer was well-intentioned, x.parent is most certainly not an argument. Therefore your suggested change would actually make the error misleading. – Ludewig

Excellent answer! I 100% agree. Put another way,throwing exceptions help developers know when they are passing incorrect inputs. I often like to consider which code (client, API, etc) and which resources are involved to understand when an exception should be handled or bubbled up. – Ariosto

This is generally a great answer, but your example of swallowing an exception might be deceiving. "The current message was aborted, and the next message can be processed as if nothing is wrong". This is often not true. Most messages will be for important reasons and you can't just continue. I'd argue that usually, shutting down the program in this case is still the right answer. – Frenzy

Answer 3

I wouldn't call it defensive programming at all - I'd call it "la la la I can't hear you" programming :) Because the code appears to be effectively ignoring a potential error condition.

Obviously we don't have any idea of what comes next in your code, but since you didn't include an else clause, I can only assume that your code just carries on even in the case that x.parent is actually null.

Bear in mind that "should not possibly be null" and "is absolutely, positively guaranteed to never be null" are not necessarily the same thing; so in that case it could lead to an exception further down the line when you try to de-reference y.

The question is then - what is more acceptable within the context of the problem you are trying to solve (the "domain") and that kind of depends on what you intend to do with ylater on.

• If y being null after this code is not a problem (let's say you do a defensive check later on for y!=null) then that's OK - although personally I don't like that style - you end up defensively checking every single de-reference because you can never be quite sure if you're a null reference away from crashing...

• If y cannot be null after the code because it will cause an exception or missing data later on, then it's simply a bad idea to continue when you know that an invariant is not correct.

Coquelicot

If y is null the code will carry on. But the consequence is that the client will get a response with important missing information. And it WILL break the client. And this is wrong in my opinion. – Preferential

I agree - hence the second point - it's a bad idea to continue on in this case. So really the "defensive" intent fails here - the correct defense here would be to fail fast with an exception. – Coquelicot

Answer 4

In short, I'd say this is NOT a defensive programming. I agree with those who thinks this code hides the system error instead of exposing and fixing. This code violates the 'fail fast' principle.

This is true if and only if x.parent is a mandatory non-null property (which seems to be obvious from the context.) However, if x.parent is an optional property (i.e. can reasonably have a null value) then this code can be allright depending on your business logic expressed.

I'm always considering usage of empty values (0, "", empty objects) instead of nulls which require tons of irrelevant if statements.

Weevil

Answer 5

After some years of thinking about this issue, I use the following "defensive" programming style - 95% of my methods return string as a success / failed result.

I return string.Empty for success, and informative text string if failed. While returning the error text, I write it to log at the same time.

public string Divide(int numA, int numB, out double division)
{
    division = 0;
    if (numB == 0)
        return Log.Write(Level.Error, "Divide - numB-[{0}] not valid.", numB);

    division = numA / numB;
    return string.Empty;
}

Then I use it:

private string Process(int numA, int numB)
{
    double division = 0;
    string result = string.Empty;
    if (!string.IsNullOrEmpty(result = Divide(numA, numB, out divide))
        return result;

    return SendResult(division);
}

When you have Log monitoring system, it lets the system show go on, but notifies the administration.

Astraphobia

"DO NOT RETURN ERROR CODES" (or error strings) blogs.msdn.microsoft.com/kcwalina/2005/03/16/… – Apuleius

You should read about Either<Left,Right> class. This is certainly improve your design. – Acoustic

After more than a decade doing something - I found how it is called - 'Railway Oriented Programming'... blog.logrocket.com/what-is-railway-oriented-programming – Astraphobia