Coding Standards – Validation of Input Parameter in Caller: Code Duplication?

coding-standardsdryvalidation

Where is the best place to validate input parameters of function: in caller or in function itself?

As I would like to improve my coding style, I try to find the best practices or some rules for this issue. When and what is better.

In my previous projects, we used to check and treat every input parameter inside the function, (for example if it is not null). Now, I have read here in some answers and also in Pragmatic Programmer book, that the validation of input parameter is responsibility of caller.

So it means, that I should validate the input parameters before calling of the function. Everywhere the function is called. And that raises one question: doesn't it create a duplication of checking condition everywhere the function is called?

I am not interested just in null conditions, but in validation of any input variables (negative value to sqrt function, divide by zero, wrong combination of state and ZIP code, or anything else)

Are there some rules how to decide where to check the input condition?

I am thinking about some arguments:

  • when the treating of invalid variable can vary, is good to validate it in caller side (e.g sqrt() function – in some case I may want to work with complex number, so I treat the condition in caller)
  • when the check condition is the same in every caller, it is better to check it inside the function, to avoid duplications
  • validation of input parameter in caller takes place only one before calling of many functions with this parameter. Therefore the validation of a parameter in each function is not effective
  • the right solution depends on the particular case

I hope this question is not duplicate of any other, I searched for this issue and I found similar questions but they don't mention exactly this case.

Best Answer

It depends. Deciding where to put validation should be based on the description and strength of the contract implied (or documented) by the method. Validation is a good way to bolster adherence to a specific contract. If for whatever reason the method has a very strict contract, then yes, it is up to you to check before calling.

This is an especially important concept when you create a public method, because you are basically advertising that some method performs some operation. It better do what you say it does!

Take the following method as an example:

public void DeletePerson(Person p)
{            
    _database.Delete(p);
}

What is the contract implied by DeletePerson? The programmer can only assume that if any Person is passed in, it will be deleted. However, we know that this isn't always true. What if p is a null value? What if p doesn't exist in the database? What if the database is disconnected? Therefore, DeletePerson does not appear to fulfill its contract well. Sometimes, it deletes a person, and sometimes it throws a NullReferenceException, or a DatabaseNotConnectedException, or sometimes it does nothing (such as if the person is already deleted).

APIs like this are notoriously difficult to use, because when you call this "black box" of a method, all sorts of terrible things can happen.

Here are a couple of ways you can improve the contract:

  • Add validation and add an exception to the contract. This makes the contract stronger, but requires that the caller perform validation. The difference, however, is that now they know their requirements. In this case I communicate this with a C# XML comment, but you could instead add a throws (Java), use an Assert, or use a contract tool like Code Contracts.

    ///<exception>ArgumentNullException</exception>
///<exception>ArgumentException</exception>
public void DeletePerson(Person p)
{            
    if(p == null)
        throw new ArgumentNullException("p");
    if(!_database.Contains(p))
        throw new ArgumentException("The Person specified is not in the database.");

    _database.Delete(p);
}

    Side note: The argument against this style is often that it causes excessive pre-validation by all calling code, but in my experience this is often not the case. Think of a scenario where you are trying to delete a null Person. How did that happen? Where did the null Person come from? If this is a UI, for example, why was the Delete key handled if there is no current selection? If it were already deleted, shouldn't it have been removed from the display already? Obviously there are exceptions to this, but as a project grows you will often thank code like this for preventing bugs to permeate deep into the system.

  • Add validation and code defensively. This makes the contract looser, because now this method does more than just deletes the person. I changed the method name to reflect this, but might not be necessary if you are consistent in your API. This approach has its pros and cons. The pro being that that you can now call TryDeletePerson passing in all sorts of invalid input and never worry about exceptions. The con, of course, is that users of your code will probably call this method too much, or it might make debugging difficult in cases where p is null. This could be considered a mild violation of the Single Responsibility Principle, so keep that mind if a flame war erupts.

    public void TryDeletePerson(Person p)
{            
    if(p == null || !_database.Contains(p))
        return;

    _database.Delete(p);
}

  • Combine approaches. Sometimes you want a little of both, where you want external callers to follow the rules closely (to force them to code responsible), but you want your private code to be flexible.

    ///<exception>ArgumentNullException</exception>
///<exception>ArgumentException</exception>
public void DeletePerson(Person p)
{            
    if(p == null)
        throw new ArgumentNullException("p");
    if(!_database.Contains(p))
        throw new ArgumentException("The Person specified is not in the database.");

    TryDeletePerson(p);
}

internal void TryDeletePerson(Person p)
{            
    if(p == null || !_database.Contains(p))
        return;

    _database.Delete(p);
}

In my experience, concentrating on the contracts you implied rather than a hard rule works best. Defensive coding appears to work better in cases where it's hard or difficult for the caller to determine whether an operation is valid. Strict contracts appear to work better where you expect the caller to only make method calls when they really, really make sense.

Related Topic