Do we need to validate entire module usage or just arguments of public methods?

Question

I've heard that it is recommended to validate arguments of public methods:

• Should one check for null if he does not expect null?
• Should a method validate its parameters?
• MSDN - CA1062: Validate arguments of public methods (I have .NET background but question is not C# specific)

Motivation is understandable. If a module will be used in a wrong way, we want to throw exception immediately instead of any unpredictable behavior.

What bothers me, is that wrong arguments is not the only error which can be made while using a module. Here's some error scenarios where we need to add checking logic if we follow recommendations and don't want error escalation:

• Incoming call - unexpected arguments
• Incoming call - module is in a wrong state
• External call - unexpected results returned
• External call - unexpected side-effects (double-entry to a calling module, breaking other dependencies states)

I've tried to take into account all these conditions and write a simple module with one method (sorry, not-C# guys):

public sealed class Room
{
    private readonly IDoorFactory _doorFactory;
    private bool _entered;
    private IDoor _door;
    public Room(IDoorFactory doorFactory)
    {
        if (doorFactory == null)
            throw new ArgumentNullException("doorFactory");
        _doorFactory = doorFactory;
    }
    public void Open()
    {
        if (_door != null)
            throw new InvalidOperationException("Room is already opened");
        if (_entered)
            throw new InvalidOperationException("Double entry is not allowed");
        _entered = true;
        _door = _doorFactory.Create();
        if (_door == null)
            throw new IncompatibleDependencyException("doorFactory");
        _door.Open();
        _entered = false;
    }
}

Now It's safe =)

It's quite creepy. But imagine how creepy it can be in a real module with dozens of methods, complex state and a lot of external calls (hi, dependency injection lovers!). Note that if you're calling a module which behavior can be overridden (non-sealed class in C#) then you're making an external call and consequences are not predictable at the scope of the caller.

Summing up, what is the right way and why? If you can choose from options below, answer additional questions, please.

Check entire module usage. Do we need unit tests? Is there examples of such code? Should dependency injection be limited in usage (since it will cause more checking logic)? Isn't it practical to move those checks to debug-time (do not include in release)?

Check only arguments. From my experience, argument checking - especially null checking - is the least effective check, because argument error rarely lead to complex mistakes and error escalations. Most of the time you'll get a NullReferenceException at the next line. So why argument checks are so special?

Don't check module usage. It's quite unpopular opinion, can you explain why?

Answer 1

TL;DR: Validate state change, rely on [validity of] current state.

_{Below I only consider release-enabled verifications. Debug-only active assertions are a form of documentation, which is useful in its own way and is out of scope for this question.}

Consider following principles:

• Common sense
• Fail fast
• DRY
• SRP

Definitions

• Component - an unit providing API
• Client - user of component's API

Mutable state

Problem

In imperative languages, error symptom and its cause may be separated by hours of heavy lifting. State corruption can hide itself and mutate to result in inexplicable failure, as inspection of current state can't reveal full process of corruption and, therefore, origin of error.

Solution

Every change of the state should be carefully crafted and verified. One way to deal with mutable state is to keep it to its minimum. This is achieved by:

• type system (const and final members declarations)
• introducing invariants
• verifying every change of component's state via public API's

When extending state of a component, consider doing so by letting compiler enforce immutability of new data. Also, enforce every sensible runtime constraint, limiting potential resulting states to a smallest possible well-defined set.

Example

// Wrong
class Natural {
    private int number;
    public Natural(int number) {
        this.number = number;
    }
    public int getInt() {
      if (number < 1)
          throw new InvalidOperationException();
      return number;
    }
}

// Right
class Natural {
    private readonly int number;
    /**
     * @param number - positive number
     */
    public Natural(int number) {
      // Going to modify state, verification is required
      if (number < 1)
        throw new ArgumentException("Natural number should be  positive: " + number);
      this.number = number;
    }
    public int getInt() {
      // State is guaranteed by construction and compiler
      return number;
    }
}

Repetition and responsibility cohesion

Problem

Checking preconditions and post-conditions of operations leads to duplication of verification code in both client and component. Validating component invocation often forces client to take some of component's responsibilities.

Solution

Rely on component to perform state verification when possible. Components are to provide an API that does not require special usage verification (arguments verification or operation sequence enforcement, for example) to keep component state to be well-defined. They oblige to verify API invocation arguments as required, report failures by means necessary, and strive to prevent their state corruption.

Clients should rely on components to verify use of their API. Not only repetition is avoided, client no longer depends on extra implementation details of component. Consider framework to be a component. Only write custom verification code when component's invariants are not strict enough or to encapsulate components exception as implementation detail.

If an operation does not change state and is not covered by state change verifications, verify every argument on deepest possible level.

Example

class Store {
  private readonly List<int> slots = new List<int>();
  public void putToSlot(int slot, int data) {
    if (slot < 0 || slot >= slots.Count) // Unnecessary, validated by List, only needed for custom error message
      throw new ArgumentException("data");
    slots[slot] = data;
  }
}

class Natural {
   int _number;
   public Natural(int number) {
       if (number < 1)
          number = 1;  //Wrong: client can't rely on argument verification, additional state uncertainity is introduced.  Right: throw new ArgumentException(number);
       _number = number;
   }
}

Answer

When described principles are applied to the example in question, we get:

public sealed class Room
{
    private bool _entered = false;
    // Do not use lazy instantiation if not absolutely necessary, this introduces additional mutable state
    private readonly IDoor _door;
    public Room(IDoorFactory doorFactory)
    {
        // Rely on system null check
        IDoor door = _doorFactory.Create();
        // Modifying own state, verification is required
        if (door == null)
           throw new ArgumentNullException("Door");
        _door = door;
    }
    public void Enter()
    {
        // Room invariants do not guarantee _entered value. Door state is indirectly a part of our state. Verification is required to prevent second door state change below.
        if (_entered)
           throw new InvalidOperationException("Double entry is not allowed");
        _entered = true;     
        // rely on immutability for _door field to be non-null
        // rely on door implementation to control resulting door state       
        _door.Open();            
    }
}

Summary

Client's state consists of own fields values and parts of component's state that are not covered by its own invariants. Verification should only be done before actual state change of a client.

Answer 2

A class is responsible for its own state. So validate to the extent that it keeps or puts things in an acceptable state.

If a module will be used in a wrong way, we want to throw exception immediately instead of any unpredictable behavior.

No, don't throw an exception, instead deliver predictable behavior. Corollary to state responsibility is to make the class/application as tolerant as practical. For example, passing null to aCollection.Add()? Just don't add and keep going. You get null input for creating an object? Create a null object or a default object. Above, the door is already open? So what, keep going. DoorFactory argument is null? Create a new one. When I create an enum I always have a Undefined member. I make liberal use of Dictionarys and enums to define things explicitly; and this goes a long way toward delivering predictable behavior.

(hi, dependency injection lovers!)

Yea though I walk through the shadow of the valley of parameters I will fear no arguments. To the preceding I also use default and optional parameters as much as possible.

All the above allows internal processing to keep going. In a particular application I have dozens of methods across multiple classes with only a single place where an exception is thrown. Even then, it's not because of null arguments or that I could not continue processing it's because the code ended up creating a "non functional"/"null" object.

edit

quoting my comment in it's entirety. I think the design should not simply "give up" when encountering 'null'. Especially using a composite object.

We're forgetting key concepts/assumptions here - encapsulation & single responsibility. There is virtually no null checking after the first, client-interacting layer. The code is tolerant robust. Classes are designed with default states and so work without being written as if interacting code is bug-ridden, rogue junk. A composite parent does not have to reach down the child layers to evaluate validity (and by implication, check for null in all the nooks and crannies). The parent knows what a child's default state means

end edit