how do i explain that if (xyz == null) checks are not "protective".

Question

i have a few developers who constantly put If null checks

For example:

Run(Order order)
{
  if (order == null) return;
}

in their code as they think they are protecting their class if someone passes in a parameter that is null. I am trying to tell them the flaw in their logic because if someone is passing in null in this case, its most likely an issue with the consumer code and instead of this class throwing an exception and failing fast, it gracefully handles the bad behavior of the consumer and keep chugging away.

another suggestion is to have precondition or guard class that fail fast and throw exceptions. any thing but ignoring that fact that the consumer probably has some other issue and i am help mask it.

how do i get folks to appreciate the fact that your class shouldn't be so forgiving. if someone doesn't pass in good data, they should be told.

any good articles or suggestions to help me get this point across?

Answer 1

It really depends on the precise situation. It's rarely advisable to give general suggestions like "don't put null checks in your code", as you seem to be indicating. The contract of the class should define what's legit and what isn't. But if the contract makes it clear that passing in null is not acceptable, then an exception is indeed an appropriate response.

Answer 2

I haven't watched through it, but eiffel.com has 2 presentations (slides+audio) on the topic of design by contract. These guys have invented the concept, so if anyone can explain it it's them :-)

Answer 3

If your class cannot accept null arguments, then the best thing to do is this:

if (arg == null)
    throw new ArgumentNullException();

This is vastly preferable to getting a NullPointerException deeper down the stack. In the worst case scenario, you'll cache that null somewhere and won't actually trigger the exception until much later, and see how much fun you'll have debugging the problem then.

And as others have stated, sometimes the contract says that null is okay. In that case, having a guard clause around some parts of the code is correct--although even then I'd say that the best design would be to add an overload without the optionally-null arguments.

Answer 4

Code contracts in .net 4.0 will hopefully make this behavior much more consistent. Any articles which talk about code contracts will help get the idea across, and in the future, this kind of syntax will provide the method.

http://blogs.msdn.com/bclteam/archive/2008/11/11/introduction-to-code-contracts-melitta-andersen.aspx

Answer 5

As everyone else has said, it is vastly preferable to fail early than to get mysterious problems in production because the function didn't do anything when it was expected to. if the function returns for null arguments, as in your example).

Even if the function doesn't return and just throws a NullReferenceException, it's eaiser to solve a bug when you know that an argument was null. If a function throws a NullReferenceException, you have no idea what was null or whose fault it was.

I'd like to add that ArgumentNullException takes a parameter for a reason.

It is better to write

if(myArg == null) throw new ArgumentNullException("myArg");

than to throw an ArgumentNullException without a paramName.

This way, if you have an exception from a function that takes five parameters, you'll know which of the parameters caused the problem. This is especially important if you cannot attach a debugger. (For example, on a production web server or an end-user machine)

If you're writing many functions, this can be a lot of overhead, especially since there's no IntelliSense for the strings. I wrote a code snippet to generate these checks:

<?xml version="1.0" encoding="utf-8" ?>
<CodeSnippets  xmlns="http://schemas.microsoft.com/VisualStudio/2005/CodeSnippet"&gt;
    <CodeSnippet Format="1.0.0">
     <Header>
      <Title>Check for null arguments</Title>
      <Shortcut>tna</Shortcut>
      <Description>Code snippet for throw new ArgumentNullException</Description>
      <Author>SLaks</Author>
      <SnippetTypes>
       <SnippetType>Expansion</SnippetType>
       <SnippetType>SurroundsWith</SnippetType>
      </SnippetTypes>
     </Header>
     <Snippet>
      <Declarations>
       <Literal>
        <ID>Parameter</ID>
        <ToolTip>Paremeter to check for null</ToolTip>
        <Default>value</Default>
       </Literal>
      </Declarations>
      <Code Language="csharp"><![CDATA[if ($Parameter$ == null) throw new ArgumentNullException("$Parameter$");
     $end$]]>
      </Code>
     </Snippet>
    </CodeSnippet>
</CodeSnippets>

Answer 6

Sometimes you can't tell people why a practice like this is wrong - they have to figure it out for themselves. But you could help them get there by coming up with some unit test that causes some nasty failure due to this problem, and make them debug the error.

Answer 7

If the method's contract specifies that its arguments should not be null, then the right thing to do is to make it explicit, by using an Assert, like this:

Debug.Assert( item != null, "Null items are not supported );

This will fail fast when the executable is built using a debug configuration, but will present zero performance degradation when built using a release configuration.

Answer 8

Well first of all, you are unequivocally incorrect. You are embracing a very serious logical fallacy. You want your code to be correct by virtue of the code assuming that everything happening around it is correct. As if correctness was some kind of magical pixie dust that you just need to spray everywhere.

All bugs are or seem stupid once they are exposed. But its checks like this that tease them out to expose themselves. Until then, bugs are invisible. And for large and complex enough projects, you don't know who will find the bug or under what conditions they will be found. Code which is architected for resilience typically has checks like this all over the place and also check the return values for every function which have to include error values. So you end up encoding a "I can't do that because the sub-functions I rely on are not working" semantic that is actually handled properly. The great value of this is that you can usually implement work arounds or self-aware debug instrumentation quite easily. Why you want to do things like this is because the most difficult bugs usually rely on both of those properties to debug properly.

Learn some lessons from your developers. They put checks like that in there, because they don't know why sometimes they get strange results from functions. You call them naive or overly cautious because of one narrow piece of knowledge that you have that they don't. But when you are debugging something nasty you are going to wonder why you don't have such checks in your code, and you are going to end up looking just as naive for not being able to spot the bug in first place.

In short: No code is made robust by assuming robustness about the environment around them.

Answer 9

My technique for avoiding if (pOrder == NULL) in C++ was to make heavy use of references

void Run(Order&);
void Run(const Order&);

no pointers, no temptation to check for NULL. But C++-style references don't exist for references in C#.

Perhaps one way of (somewhat) mimicing the C++ technique in C# is to use struct Order instead of class Order (see use MORE structs?). You could then have

void Run(Order order) { ... work on a COPY of 'order' ... }
void Run(ref Order order) { ... }

if you really wanted to pass null to Run(), you could do that too:

void Run(Order? order) { ... }

Answer 10

This seems to be a question about how best write code that is manageable. It is my new belief that you must assume ignorance of all consumers of your code. I have gotten myself into trouble by assuming I or someone with deep knowledge would be consuming my code. The only thing I would add to throwing an exception is creating custom exceptions as well as leaving breadcrumbs in the inner exception. I believe strongly in giving your developers a chance to run down the issue especially if it is due to data. I spend most of my time looking for the data that breaks my code and if you can leave hints you will save weeks in a year.