Best practices when validating function arguments for public/private methods

Question

Doing some research, it seems that people generally agree that arguments to public methods should be validated while private functions typically do not. This causes me to have some questions, but I have not been able to find a satisfactory answer so far.

Example:

public void DoSomething(int i)
{
    if (i < 0)
        throw new ArgumentOutOfRangeException("i");

    double d = DoWork(i);
}

private double DoWork(int i)
{
    double ret = ...; // some calculation
    return ret;
}

Thoughts:

1. What if the requirement of i to be non-negative changes inside DoWork()? The design risks leaving outdated validation checks in place. The programmer is responsible for adjusting the usage of a function that has changed, I know, but it leaves me wondering if there is a better way to minimize risk of error.

2. What about different calls to DoWork() not from DoSomething()? Must we validate the arguments redundantly?

public void DoSomething(int i)
{
    if (i < 0)
        throw new ArgumentOutOfRangeException("i");

    double d = DoWork(i);
}

public void DoSomethingElse()
{
    int i = 5;
    if (i < 0)
        throw new ArgumentOutOfRangeException("i");

    double d = DoWork(i);
}

private double DoWork(int i)
{
    double ret = ...; // some calculation
    return ret;
}

This can be cleaned up a little by putting the check into a function of its own. Then there is the risk that a new function making a call to DoWork(int i) will forget to validate i.

public void DoSomething(int i)
{
    ThrowIfIntegerIsNegative(i);

    double d = DoWork(i);
}

public void DoSomethingElse()
{
    int i = 5;
    ThrowIfIntegerIsNegative(i);

    double d = DoWork(i);
}

static void ThrowIfIntegerIsNegative(int i)
{
    if (i < 0)
        throw new ArgumentOutOfRangeException("i");
}

private double DoWork(int i)
{
    double ret = ...; // some calculation
    return ret;
}

Is that at all better than this?

public void DoSomething(int i)
{
    double d = DoWork(i);
}

public void DoSomethingElse()
{
    double d = DoWork(5);
}

private double DoWork(int i)
{
    if (i < 0)
        throw new ArgumentOutOfRangeException("i");

    double ret = ...; // some calculation
    return ret;
}

Depending on the situation, these are some goals I am trying to achieve simultaneously:

1. Have the argument validation in a single place (and possibly inside the function using the argument)
2. Report an error sooner rather than later (might not want to have a bunch of code run for an hour just to fail at the end for some bad user input)
3. Avoid validating arguments multiple times
4. Avoid performance impact in release code

How do you strike a balance? What methodology has worked best for you? I would greatly appreciate any insight.

Answer 1

The logic behind validating arguments in public methods and not validating arguments in private ones goes roughly as follows:

• When a public method is called with invalid arguments, it is a programming error outside your control.
• When a non-public method is called with invalid arguments, it is a logical error within your control.

This logic is reasonable: there is no need to waste cycles on validating the arguments to methods that your module produces internally. Private methods, on the other hand, could always assume that their arguments are valid, because you are in control of all calls of private methods.

However, it is very beneficial to catch situations when these assumptions are violated. To that end, it is a very good idea to use run-time assertions in place of argument validation inside private methods. This catches invalid invocations from outside callers with exceptions, and alerts you to invalid invocations from your own methods with asserts.

Answer 2

I like to validate with exceptions in public methods:

public void Foo(int i)
{
    if (i < 0)
      throw new ArgumentOutOfRangeException("i");

    double d = DoWork(i);
}

And I like to validate with Asserts in private methods:

private double DoWork(int i)
{
    System.Diagnostics.Debug.Assert(i >= 0);
    // ....
}

1. It means duplicate validation in multiple public methods.
2. It catches the invalid parameters as early as possible.
3. Throwing an ArgumentOutOfRangeException exception due to invalid parameter inside the private method may be confusing to the caller of the public method due to differences in parameter names, etc.
4. Often, your public methods do additional work before calling your private methods, so that work is saved.
5. Use unit testing to ensure new methods don't miss any validation checks.

One additional thing to do is treat any protected method like a public method.

Edit:

This is an example of bad validation which violates #3 above:

public void Foo(int distanceInMetres)
{
    double d = DoWork(distanceInMetres * 1000);
}

private double DoWork(int distanceInMillimetres)
{
    if (distanceInMillimetres < 0)
      throw new ArgumentOutOfRangeException("distanceInMillimetres");
    // ....
}

If the caller to Foo sees the exception on the parameter "distanceInMillimetres", he will be confused because he called "Foo" which took a parameter named "distanceInMetres".

Answer 3

Agree with dasblinkenlight statements about public and not public methods.

If you practice TDD, your private methods is just the result of refactoring code and extracting some pieces of public methods into private methods. So if you covered public methods with tests, your private methods will be automatically tested.

If you don't use TDD but want to verify that logic is not violated, you can use different techniques, for example - asserts (http://msdn.microsoft.com/en-us/library/ttcc4x86.aspx) or code contracts (http://msdn.microsoft.com/en-us/library/dd264808.aspx).