Validating arguments of private methods because of Reflection in C#

Question

I am wondering whether it is necessary to validate once more the arguments passed to private methods of classes which without using Reflection would only be called by public methods in the same class.

If the private method instantiates an object that needs to be disposed (before something goes wrong because of bad arguments), an exception could be thrown (in which case the object would be disposed anyway), right?

I was viewing some of the source code of .NET (mainly the String and Stream classes). I found that some private methods arguments are verified with contracts, but in others no occur check.

Code that does not validate once more the arguments (taken from the String class). In this case a NullReferenceException can be thrown because of the trimChars argument.

    [System.Security.SecuritySafeCritical]  // auto-generated
    private String TrimHelper(char[] trimChars, int trimType) {
        //end will point to the first non-trimmed character on the right
        //start will point to the first non-trimmed character on the Left
        int end = this.Length-1;
        int start=0;

        //Trim specified characters.
        if (trimType !=TrimTail)  {
            for (start=0; start < this.Length; start++) {
                int i = 0;
                char ch = this[start];
                for( i = 0; i < trimChars.Length; i++) {
        // ... more code

A code that validates the arguments twice (once in public and once in private method) is this (taken from Stream class).

   [HostProtection(ExternalThreading = true)]
   [ComVisible(false)]
   public virtual Task CopyToAsync(Stream destination, Int32 bufferSize, CancellationToken cancellationToken)
   {
      if (destination == null)
         throw new ArgumentNullException("destination");
         // ... more code goes here
      return CopyToAsyncInternal(destination, bufferSize, cancellationToken);
   } 


    private async Task CopyToAsyncInternal(Stream destination, Int32 bufferSize, CancellationToken cancellationToken)
    {
        Contract.Requires(destination != null);
        // ... more code
    }

Which is the best practice, or does it depend on the situation - whether it is a class library that is only used in a specific project or it would be used in different contexts that can't be known in advance?

Answer 1

This is not a full answer to your question, but it's too long for a comment:

Contract.Requires is not the same as validating a parameter and throwing an exception if the validation fails.

Unless you configure Code Contracts to use run-time checks the Contract.Requires won't even make it into the final assembly code (same Stream example decompiled using dotPeek)

private Task CopyToAsyncInternal(Stream destination, int bufferSize, CancellationToken cancellationToken)
{
    Stream.<CopyToAsyncInternal>d__2 stateMachine;
    stateMachine.<>__this = this;
    stateMachine.destination = destination;

There is no validation in there and the Stream.<CopyToAsyncInternal>d__2 does not validate its destination field in any way before accessing it in its MoveNext method.

But using Contract.Requires in private methods allows for a better static analysis of contracts.

Answer 2

I am wondering whether it is necessary to validate once more the arguments passed to private methods of classes which without using Reflection would only be called by public methods in the same class.

That mainly depends on whether you support third-party callers using reflection to call those methods. Most people don't, and don't design their code to support it. I also don't, and don't recommend it.

Would you worry about breaking third-party callers using reflection when you rename a private method? If not, then don't worry about the code in those methods either.

But see below.

If the private method instantiates an object that needs to be disposed (before something goes wrong because of bad arguments), an exception could be thrown (in which case the object would be disposed anyway), right?

No. Dispose() is a method that many classes implement, usually along with the IDisposable interface, but for the runtime, neither this interface nor the method has any specific meaning.

If an object is instantiated, and there are no references to that object, it will (at some point) be garbage-collected, at which point the class's destructor (aka finalizer) runs. There's no rule that says the Dispose() method and the destructor must do the same thing, and there are sometimes good reasons why they don't do the same thing. But if you have any unmanaged resources, then almost certainly, both will release those resources.

Which is the best practice, or does it depend on the situation - whether it is a class library that is only used in a specific project or it would be used in different contexts that can't be known in advance?

Well, one reason it can make sense to do contract validation in private methods, is if you move the contract validation out of the public methods.

Given

public void Foo(string x) { Baz(x, false); }
public void Bar(string x) { Baz(x, true); }
private void Baz(string x, bool y) {
  if (x == null) throw new ArgumentNullException("x");
  // ...
}

it may make sense to put the contract validation in Baz, to reduce code duplication.

Another time it may make sense to put contract validation in private methods is if you have external tools that do static contract checking (that'll mainly be Code Contracts), and the contract checker is otherwise unable to verify that your method's body doesn't violate any other method's contract:

private void Foo(string x) { Bar(x); }

If the contract checker sees that Bar requires x to be non-null, and is unable to verify that all calls to Foo will pass a non-null value, it may produce a warning inside Foo. (Or it may produce a warning inside Foo's caller. It depends.)

Of course, if Code Contracts is used, then the earlier removal of code duplication is a bad idea: Code Contracts does need the contracts to be present in the public methods.