User Input Validation – How to Handle Invalid Input

Invalid null pointers can either be caused by programmer error or by runtime error. Runtime errors are something a programmer can't fix, like a malloc failing due to low memory or the network dropping a packet or the user entering something stupid. Programmer errors are caused by a programmer using the function incorrectly.

The general rule of thumb I've seen is that runtime errors should always be checked, but programmer errors don't have to be checked every time. Let's say some idiot programmer directly called graph_get_current_column_color(0). It will segfault the first time it's called, but once you fix it, the fix is compiled in permanently. No need to check every single time it's run.

Sometimes, especially in third party libraries, you'll see an assert to check for the programmer errors instead of an if statement. That allows you to compile in the checks during development, and leave them out in production code. I've also occasionally seen gratuitous checks where the source of the potential programmer error is far removed from the symptom.

Obviously, you can always find someone more pedantic, but most C programmers I know favor less cluttered code over code that is marginally safer. And "safer" is a subjective term. A blatant segfault during development is preferable to a subtle corruption error in the field.

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.