code contracts/asserts: what with duplicate checks?

Question

I'm a huge fan of writing asserts, contracts or whatever type of checks available in the language I'm using. One thing that bothers me a bit is that I'm not sure what the common practice is for dealing with duplicate checks.

Example situation: I first write the following function

void DoSomething( object obj )
{
  Contract.Requires<ArgumentNullException>( obj != null );
  //code using obj
}

then a few hours later I write another function that calls the first one. As everything is still fresh in memory, I decide not to duplicate the contract, since I know that DoSomething wil check for a null object already:

void DoSomethingElse( object obj )
{
  //no Requires here: DoSomething will do that already
  DoSomething( obj );
  //code using obj
}

The obvious problem: DoSomethingElse now depends on DoSomething for verifying that obj is not null. So should DoSomething ever decide not to check anymore, or if I decide to use another function obj might not be checked anymore. Which leads me to writing this implementation after all:

void DoSomethingElse( object obj )
{
  Contract.Requires<ArgumentNullException>( obj != null );
  DoSomething( obj );
  //code using obj
}

Always safe, no worries, except that if the situation grows the same object might be checked a number of times and it's a form of duplication and we all know that's not so good.

What is the most common practice for situation like these?

Answer 1

Personally I would check for null in any function that will fail if it gets a null, and not in any function which won't.

So in your example above, if doSomethingElse() doesn't need to dereference obj then I wouldn't check obj for null there.

If DoSomething() does dereference obj then it should check for null.

If both functions dereference it then they should both check. So if DoSomethingElse dereferences obj then it should check for null, but DoSomething should also still check for null as it may be called from another path.

This way you can leave the code fairly clean and still guarantee that checks are in the correct place.

Answer 2

Great! I see you found out about Code Contracts for .NET. Code Contracts goes a lot further than your average assertions, of which the static checker is the best example. This might not be available to you if you haven't got Visual Studio Premium or higher installed, but it is important to understand the intention behind it if you are going to use Code Contracts.

When you apply a contract to a function, it literally is a contract. That function guarantees to behave according to the contract, and is guaranteed to only be used as defined by the contract.

In your given example, the DoSomethingElse() function doesn't live up to the contract as specified by DoSomething(), as null can be passed, and the static checker will indicate this problem. The way to resolve this is by adding the same contract to DoSomethingElse().

Now, this means there will be duplication, but this duplication is necessary as you choose to expose the functionality across two functions. These functions, although private, can be called from different places in your class as well, so the only way to guarantee that from any given call the argument will never be null is to duplicate the contracts.

This should make you reconsider why you split the behavior in two functions in the first place. It has always been my opinion (contrary to popular belief) that you shouldn't split functions which are only called from one place. By exposing the encapsulation by applying the contracts this becomes even more evident. It seems I found an extra argumentation for my cause! Thank you! :)