Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
3  
ArgumentBullException? That's a new one :) –  Michael Kjörling Nov 7 '11 at 12:00
    
lol @ my typing skills... I'll edit it. –  stijn Nov 7 '11 at 13:09
add comment

2 Answers

up vote 11 down vote accepted

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.

share|improve this answer
    
I fully agree. Each method's preconditions should stand on their own. Imagine that you rewrite DoSomething() such that the precondition is no longer required (unlikely in this particular case, but might happen in a different situation), and remove the precondition check. Now some seemingly totally unrelated method is broken because of the missing precondition. I'll take a slight bit of code duplication for clarity over weird failures like that from a desire to save a few lines of code, any day. –  Michael Kjörling Nov 7 '11 at 12:02
add comment

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! :)

share|improve this answer
    
regarding your last paragraph: in the actual code both functions were members of two different classes, which is why they are split. Apart from that, I was in the following situation so many times: write a lengthy function, decide not to split it. Later figure out that some of the logic is duplicated somewhere else, so split it anyway. Or a year later read it again and find it unreadable, so split it anyway. Or when debugging: split functions = less banging on the F10 key. There's more reasons, so personally I prefer splitting, even though it means it might be too extreme sometimes. –  stijn Nov 9 '11 at 9:14
    
(1) "Later figure out that some of the logic is duplicated somewhere else". That is why I find it more important to always "develop towards an API" than to simply split functions. Constantly think about reuse, not only within the current class. (2) "Or a year later read it again and find it unreadable" Because functions have names this is better? You have even more readability if you use what a commenter on my blog described as "code paragraphs". (3) "split functions = less banging on the F10 key" ... I don't see why. –  Steven Jeuris Nov 9 '11 at 9:46
    
(1) agreed (2) readability is personal preference so that's not really something under discussion for me.. (3) going through a function of 20 lines requires hitting F10 20 times. Going through a functions that has 10 of those lines in a split function requires gives me the choice of only having to hit F10 11 times. Yes, in the first case I can put breakpoints or select 'jumpt to cursor' but that's still more of an effort than the second case. –  stijn Nov 9 '11 at 9:53
    
@stijn: (2) agreed ;p (3) thanks for clarifying! –  Steven Jeuris Nov 9 '11 at 10:06
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.