Do you throw an argumentexception or argumentnullexception from private methods?

Question

I was just reviewing some code I wrote a while back, and can see that I have a couple of private methods that throw argumentnullexceptions and/or argumentexceptions if there are issues with the methods parameters.

I guess my rationale is that helps future proof the application if someone attempts to "misuse" the method in future. However, given it is a private method and the people who are likely to call this method can see the associated comments and code, it is just unnecessary to throw this. It certainly doesn't hurt to have them, although it does add clutter.

My feeling is that these exceptions are generally more useful on something like a API that is going to be exposed publically.

Answer 1

Normally, for private methods you do not throw exceptions since as you wrote it, the developer is supposed to know how and where he is calling the method from. As such, the variables passed as parameters to the private method should be checked outside of the method, that is, before calling it. Throwing "InvalidArgumentException" and other such exceptions is considered good practice for public methods (whether you are writing an "API" or not).

For those cases where you want to throw "InvalidArgumentException" it's worth mentioning that there is an Assert class in the Spring API for Java since version 1.1.2. It has been very helpful - to me at least - in writing less code to perform checks.

You may however use "asserts" to check parameters in private methods. That is one of their true purpose. They are more reasons to use them, check out the following link which also explains thoroughly when to use asserts and when to use exceptions. Asserts are not to be included in production code and the compiler removes them by default. So they are what you are looking for: helping the developers, invisible for the users. In Java you have to use a special flag ("-ea") to tell the compiler to enable assertions. You may consider them as "debugging" friends.

Here is how to use asserts in:

• Java: http://download.oracle.com/javase/1.4.2/docs/guide/lang/assert.html
• .NET: http://msdn.microsoft.com/en-us/library/system.diagnostics.debug.assert(v=VS.100).aspx

Answer 2

Like everything else it depends....

If the public methods are simple wrappers that call the private method (along the lines of a private overloaded method) then it may make sense to throw an exception in the private method instead of checking in each public one.

Generally if it doesn't meet the above definition then I wouldn't usually check the arguments / throw an exception on a private method. Although there are other cases I generally do this in a private method prior to performing some expensive operation that could fail part way through if the arguments are invalid.

Answer 3

I realize that while the question has no language tag, it is probably implicitly talking about “coffee languages”. But just for the sake of completeness, I'd like to mention the somewhat diverging apparent consensus in the C++ world.

There are three things C++ programmers will typically be interested in:

• Will it have zero-overhead in optimized builds? (That is, can it be “compiled out”?)
• Can I use it to trap into a debugger right at the point where the error was detected?
• Can I use it to report problems from functions declared noexcept?

In the past, I have approached the first problem by writing code like this

int
factorial(const int n)
{
  if (CHECK_ARGS)
    {
      if (n < 0)
        throw std::invalid_argument {"n < 0"};
    }
  int fac = 1;
  for (int i = 2; i <= n; ++i)
    fac *= i;
  return fac;
}

where CHECK_ARGS is #defined to a compile-time constant so the compiler may completely eliminate all the argument checking code in optimized builds. (I'm not saying that compiling the checks out is a good thing in general but I do believe that a user should have the option to compile them out.)

I still like about this solution that the argument checking code is clearly visible grouped together into the if. However, the second and third issue are not solved by this. Therefore, I'm now leaning again more towards using an assert macro for argument checking.

The Boost coding standards agree with this:

What About Programmer Errors?

As a developer, if I have violated a precondition of a library I'm using, I don't want stack unwinding. What I want is a core dump or the equivalent - a way to inspect the state of the program at the exact point where the problem was detected. That usually means assert() or something like it.

There was a very interesting talk given by John Lakos at CppCon'14 titled Defensive Programming Done Right (part 1, part 2). In the first part of his talk, he discusses the theory of contracts and undefined behavior. In the second part, he presents what I consider a very good proposal for systematic argument checking. In essence, he proposes assertion macros that allow the user to select how much of a budget (in terms of CPU utilization) she is willing to donate to the library for argument checking and has the library make wise use of that budget. As an addition, the user can also install a global error handling function that will be called in case a broken contract is detected.

Regarding the aspect that a function is private, I don't think that this means we should never have it check its arguments. We might trust our own code more to not violate an internal function's contract but we also know that we aren't perfect either. Argument checking in internal functions is just as helpful in detecting our own bugs as it is in public functions for detecting bugs in client code.

Answer 4

Consider the following structure:

1. Internal logic: This function assumes to be called with correct parameters and therefor uses asserts to verify preconditions, postconditions and invariants to check your inner logic.

2. User interface wrapper: This function wraps the internal function and uses InvalidArgumentExceptions to handle wrong values and to tell the user to correct his inputs: Assert(x).hasLength(4);, Assume(y).isAlphanumeric();, Assert(z).isZipCode();, Assume(mailAdress).matchesRegex(regex_MailAdress);, Reject(x).ifEmpty();, etc.

3. Batch interface wrapper: This function wraps the internal function and uses logging, validity markings and statistics to handle wrong values without interrupting some long-running task. The markings could be used later by someone checking and cleaning the result-database.

4. Command line interface wrapper: this function wraps the internal function and asks again for the last input.

You should use both - asserts and exceptions - in different methods for different tasks. You should separate internal logic from parameter checking. Compare it to the separation of Model, View, Controller.

Answer 5

There are better ways to avoid null reference check: use code contract or an AOP framework to do the check for you. Google "c# code contract" or "postsharp".