Validation of the input parameter in caller: code duplication?

Question

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.

Answer 1

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.

Answer 2

It's a matter of convention, documentation and use case.

Not all functions are equal. Not all requirements are equal. Not all validation is equal.

For example, if your Java project tries to avoid null pointers whenever possible (see the Guava style recommendations, for example), do you still validate every function argument to make sure it is not null? It's probably not necessary, but chances are that you still do it, to make it easier to find bugs. But you may use an assert where you previously threw a NullPointerException.

What if the project is in C++? Convention/tradition in C++ is to document preconditions, but only verify them (if at all) in debug builds.

In either case, you have a documented precondition on your function: no argument may be null. You could instead extend the domain of the function to include nulls with defined behavior, e.g. "if any argument is null, throws an exception". Of course, that's again my C++ heritage speaking here - in Java, it's common enough to document preconditions this way.

But not all preconditions even can be reasonably checked. For example, a binary search algorithm has the precondition that the sequence to be searched must be sorted. But verifying that it definitely is so is an O(N) operation, so doing that on every call kinda defeats the point of using an O(log(N)) algorithm in the first place. If you're programming defensively, you can do lesser checks (e.g. verifying that for each partition you search, the start, mid and end values are sorted), but that doesn't catch all errors. Typically, you will just have to rely on the precondition being fulfilled.

The one real place where you need explicit checks is at boundaries. External input to your project? Validate, validate, validate. A gray area is API boundaries. It really depends on how much you want to trust the client code, how much damage invalid input does, and how much assistance you want to provide in finding bugs. Any privilege boundary must count as external, of course - syscalls, for example, run in an elevated privilege context and therefore must be very careful to validate. Any such validation must of course be internal to the syscall.

Answer 3

Parameter validation should be the concern of the function being called. The function should know what is considered valid input and what is not. Callers may not know this, especially when they don't know how the function is implemented internally. The function should be expected to handle any combination of parameter values from callers.

Because the function is responsible for validating parameters, you can write unit tests against this function to ensure it behaves as intended with both valid and invalid parameter values.

Answer 4

Within the function itself. If the function is used more than once, you wouldn't want to verify the parameter for every function call.

Moreover, if the function is updated in such way that will affect the validation of the parameter, you have to search for every occurrence of the caller validation to update them. It ain't lovely :-).

You may refer to Guard Clause

Update

See my reply for each scenario you've provided.

• 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)

  Answer

  Majority of programming languages supports integer and real numbers by default, not complex number, hence their implementation of sqrt only accepts non-negative number. The only case you have a sqrt function that returns complex number is when you use programming language oriented towards math, like Mathematica

  Moreover, sqrt for most programming languages is already implemented, hence you couldn't modify it, and if you try to replace the implementation (see monkey patching), then your collaborators will be utterly shock on why sqrt suddenly accepts negative numbers.

  If you wanted one you can wrap it around your custom sqrt function that handles negative number and returns complex number.

• when the check condition is the same in every caller, it is better to check it inside the function, to avoid duplications

  Answer

  Yes, this is a good practice to avoid scattering the parameter validation across your code.

• 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

  Answer

  It will be nice if the caller is a function, don't you think?

  If the functions within the caller are used by other caller, what prevents you from validating the parameter within the functions called by the caller?

• the right solution depends on the particular case

  Answer

  Aim for maintainable code. Moving your parameter validation ensures one source of truth on what the function can accept or not.

Answer 5

A function should state its pre- and post-conditions.
The pre-conditions are the conditions that must be met by the caller before it can correctly use the function and can (and often do) include the validity of input parameters.
The post-conditions are the promises that the function makes to its callers.

When the validity of a function's parameters is part of the pre-conditions, then it is the caller's responsibility to ensure those parameters are valid. But that does not mean every caller has to explicitly check each parameter prior to the call. In most cases, no explicit tests are needed because the internal logic and the pre-conditions of the caller already ensure that the parameters are valid.

As a safety measure against programming errors (bugs), you can check that the parameters passed in to a function really do meet the stated pre-conditions. As these tests can be costly, it is a good idea to be able to switch them off for release builds. If these tests fail, then the program should be terminated, because it has provably run into a bug.

Although at first glance the check in the caller seems to invite code duplication, it is actually the other way around. The check in the callee results in code duplication and lots of unneeded work being done.
Just think about it, how often do you pass parameters through several layers of functions, making only small changes to some of them along the way. If you consistently apply the check-in-callee method, each of those intermediate functions will have to re-do the check for each of the parameters.
And now imagine that one of those parameters is supposed to be a sorted list.
With the check in the caller, only the first function would have to make sure that list is really sorted. All the others know the list is already sorted (as that is what they stated in their pre-condition) and can pass it on without further checks.

Answer 6

Most often you can't know who, when and how will call the function you wrote. It is best to assume the worst: your function will be called with invalid parameters. So you should definitely cover that.

Nevertheless, if the language you use supports exceptions, you might not check for certain errors and be sure that an exception will be thrown, but in this case you must be sure to describe the case in the documentation (you need to have documentation). The exception will give the caller enough information about what happened, and will also drive attention to the invalid arguments.