When to check function/method parameters?

Question

Hi SO-lers,

when writing small functions I often have the case that some parameters are given to a function which itself only passes them to different small functions to serve its purpose.

For example (C#-ish Syntax):

public void FunctionA(object param)
{
    DoA(param);
    DoB(param);
    DoC(param);
    // etc.
}

private void DoA(object param)
{
    DoD(param);
}

private void DoD(object param)
{
    // Error if param == null
    param.DoX();
}

So the parameters are not used inside the called function but "somewhere" in the depths of the small functions that do the job.

So when is it best to check if my param-Object is null?

When checking in FunctionA:

Pro: -There is no overhead through the use of further methods which at last will do nothing because object is null.

Con: -My syntactically wonderful FunctionA is dirtied by ugly validation code.

When checking only when param-object is used:

Pro: -My syntactically wonderful FunctionA keeps a joy to read :)

Cons: -There will be overhead through calling methods which will do nothing because the param-object is null. -Further cons I don't think about at the moment.

Answer 1

Unless you think the value is likely to be null the vast majority of the time, I'd put the validation in DoD(). If you put it in FunctionA() you'll have to repeat the validation code later when you decide FunctionB() also needs to use DoD(). To me, the extra overhead is worth not having to repeat myself.

Answer 2

Always put it as far down the call stack as possible, so that if you later refactor the code and something else calls DoD other than DoA you have the check in place and don't have to rework your parameter checks. The overhead of a small null check and possibly a few extra method calls is going to be trivial in most cases and doing the check an extra few times is not something you should be worrying about.

Answer 3

As a guideline, I make a habit of it to check every parameter that is used by the method, even including my own private variables. I would therefore only check for nil in your DoD method.

You might want to check out Bertrand Meyers Design By Contract mantra.

Answer 4

Always check everything :) Coming from the deep bowels of coding libraries for embedded systems, this is the method I'd use:

public void FunctionA(object param)
{
    assert(param != null && param.canDoX());
    DoA(param);
    DoB(param);
    DoC(param);
    // etc.
}

private void DoA(object param)
{
    assert(param != null && param.canDoX());
    DoD(param);
}

private void DoD(object param)
{
    assert(param != null && param.canDoX());
    if ( param != null )
        param.DoX();
    else
        // Signal error, for instance by throwing a runtime exception
        // This error-handling is then verified by a unit test that 
        // uses a release build of the code.
}

To de-clutter this, the obvious solution is to break out the validation to a separate validator function. Using a C-style preprocessor, or just sticking to asserts, it should be trivial to have this "paranoid" validation excluded from release builds.

Answer 5

Fail early. Unless a partial result is preferable to no result at all, execution should stop as soon as the code can detect that there is a problem. Why should the code run through several methods when the result downstream is going to be an invalid or missing argument?

If it is possible that the downstream methods could be called separately then validation could be handled by a call to ca common validation methed as has already been suggested.

Answer 6

It's the caller's responsibility to pass a valid parameter. In this case:

if(param != null)
{
  FunctionA(param);
}