29

I use exceptions to catch problems early. For example:

public int getAverageAge(Person p1, Person p2){
    if(p1 == null || p2 == null)
        throw new IllegalArgumentException("One or more of input persons is null").
    return (p1.getAge() + p2.getAge()) / 2;
}

My program should never pass null in this function. I never intend it to. However as we all know, unintended stuff happens in programming.

Throwing an exception if this problem occurs, allows me to spot and fix it, before it causes more problems in other places in the program. The exception stops the program and tells me "bad stuff happened here, fix it". Instead of this null moving around the program causing problems elsewhere.

Now, you're right, in this case the null would simply cause a NullPointerException right away, so it might not be the best example.

But consider a method such as this one for example:

public void registerPerson(Person person){
    persons.add(person);
    notifyRegisterObservers(person); // sends the person object to all kinds of objects.
}

In this case, a null as the parameter would be passed around the program, and might cause errors much later, which will be hard to trace back to their origin.

Changing the function like so:

public void registerPerson(Person person){
    if(person == null) throw new IllegalArgumentException("Input person is null.");
    persons.add(person);
    notifyRegisterObservers(person); // sends the person object to all kinds of objects.
}

Allows me to spot the problem much before it causes weird errors in other places.

Also, a null reference as a parameter is only an example. It could be many kinds of problems, from invalid arguments to anything else. It's always better to spot them early.


So my question is simply: is this good practice? Is my use of exceptions as problem-preventing tools good? Is this a legitimate application of exceptions or is it problematic?

  • 2
    Can the downvoter explain what is wrong with this question? – Giorgio Sep 14 '14 at 20:37
  • 2
    "crash early, crash often" – Bryan Chen Sep 14 '14 at 21:58
  • 16
    That is literally what exceptions are there for. – raptortech97 Sep 15 '14 at 12:56
  • Note that coding contracts enable you to detect many such errors statically. This is generally superior to throwing an exception at runtime. The support and effectiveness of coding contracts varies by language. – Brian Sep 15 '14 at 21:10
  • 3
    Since you are throwing IllegalArgumentException, it is redundant to say "Input person". – kevin cline Sep 15 '14 at 23:00
29

Yes, "fail early" is a very good principle, and this is simply one possible way of implementing it. And in methods that have to return a specific value, there isn't really very much else you can do to fail deliberately - it's either throwing exceptions, or triggering assertions. Exceptions are supposed to signal 'exceptional' conditions, and detecting a programming error certainly is exceptional.

  • Fail early is the way to go if there is no way to recover from an error or condition. For example if you need to copy a file and the source or destination path is empty then you should throw an exception right away. – Michael Shopsin Sep 15 '14 at 18:27
  • It is also known as "fail fast" – keuleJ Sep 16 '14 at 4:47
8

Yes, throwing exceptions is a good idea. Throw them early, throw them often, throw them eagerly.

I know there's an "exceptions vs. assertions" debate, with some kinds of exceptional behavior (specifically, the ones thought to reflect programming errors) handled by assertions that can be "compiled out" for runtime rather than debugging/testing builds. But the amount of performance consumed in a few extra correctness checks is minimal on modern hardware, and any extra cost is far outweighed by the value of having correct, uncorrupted results. I've never actually met an application codebase for which I'd want (most) checks removed at runtime.

I'm tempted to say I wouldn't want a lot of extra checking and conditionals within the tight loops of numerically intensive code...but that's actually where a lot of numerical errors are generated, and if uncaught there, will propagate outward to effect all results. So even there, checks are worth doing. In fact, some of the best, most efficient numerical algorithms are based on error evaluation.

One final place to be very conscious of extra code is very latency-sensitive code, where extra conditionals can cause pipeline stalls. So, in the middle of operating system, DBMS, and other middleware kernels, and low-level communication/protocol handling. But again, those are some of the places errors are most likely to be observed, and their (security, correctness, and data integrity) effects to be most damaging.

One improvement I have found is to not throw only base-level exceptions. IllegalArgumentException is good, but it can come from essentially anywhere. It doesn't take much in most languages to add custom exceptions. For your person-handling module, say:

public class PersonArgumentException extends IllegalArgumentException {
    public MyException(String message) {
        super(message);
    }
}

Then when someone sees a PersonArgumentException, it's clear where it's coming from. There's a balancing act about how many custom exceptions you want to add, because you don't want to multiply entities unnecessarily (Occam's Razor). Often just a few custom exceptions are enough to signal "this module isn't getting the right data!" or "this module can't do what it's supposed to do!" in a way that is specific and tailored, but not so over-precise that you have to re-implement the entire exception hierarchy. I often come to that small set of custom exceptions by starting with stock exceptions, then scanning the code and realizing that "these N places are raising stock exceptions, but they boil down to the higher-level idea that they're not getting the data they need; let's replace those stock exceptions with a higher-level exception that more clearly communicates what's really going on."

  • I've never actually met an application codebase for which I'd want (most) checks removed at runtime. Then you haven't done code that's performance-critical. I'm working on something right now that does 37M ops per second with assertions compiled in on and 42M without them. The assertions aren't there validate outside input, they're there to make sure the code is correct. My customers are more than happy to take the 13% increase once I'm satisfied that my stuff isn't broken. – Blrfl Sep 23 '14 at 16:54
  • Peppering a codebase with custom exceptions should be avoided because while it may help you, it doesn't help others who have to get acquainted with them. Usually if you find yourself extending a common exception and not adding any members or functionality, you don't actually need it. Even in your example,PersonArgumentException isn't as clear as IllegalArgumentException. The latter is universally known to be thrown when an illegal argument is passed. I'd actually expect the former to be thrown if a Person was in an invalid state for a call (Similar to InvalidOperationException in C#). – Selali Adobor Sep 23 '14 at 17:26
  • It's true that if you're not careful you can trigger the same exception from somewhere else in the function, but that's a flaw of the code, not of the nature of common exceptions. And that's where debugging and stack traces come in. If you're trying to "label" your exceptions, then use the existing facilities that most common exceptions provide, like using the message constructor (that's exactly what it's there for). Some exceptions even provide constructors with additional parameters to get the name of the problematic argument, but you can provide the same facility with a well formed message. – Selali Adobor Sep 23 '14 at 17:26
  • I admit that merely rebranding a common exception to a private label of basically the same exception is a weak example, and rarely worth the effort. In practice I try to emit custom exceptions at a higher semantic level, more intimately tied to the intent of the module. – Jonathan Eunice Sep 25 '14 at 14:19
  • I've done performance-sensitive work in the numerical/HPC and OS/middleware realms. A 13% performance bump is no small thing. I might turn off some checks to get it, in the same way a military commander might ask for 105% of nominal reactor output. But I've seen "it will run faster this way" given often as a reason to turn off checks, backups, and other protections. It's basically trading off resiliency and safety (in many cases, including data integrity) for extra performance. Whether that's worthwhile is a judgment call. – Jonathan Eunice Sep 25 '14 at 14:34
3

Failing as soon as possible is great when debugging an application. I remember a particular segmentation fault in a legacy C++ program : the place where the bug was detected had nothing to do with the place where it was introduced (the null pointer was happily moved from one place to another in memory before it finally caused a problem). Stack traces cannot help you in those cases.

So yes, defensive programming is a really effective approach to detect and fix bugs quickly. On the other hand, it can be overdone, especially with null references.

In your specific case, for example: if any of the reference is null, the NullReferenceException will be thrown at the next statement, when trying to get the age of one person. You don't really need to check things by yourself here: let the underlying system catch those errors and throw exceptions, that's why they exist.

For more realistic example, you can use assert statements, which :

  1. Are shorter to write and read:

        assert p1 : "p1 is null";
        assert p2 : "p2 is null";
    
  2. Are specifically designed for your approach. In a world where you have both assertions and exceptions, you can distinguish them as follows:

    • Assertions are for programming errors ("/* this should never happen */"),
    • Exceptions are for corner cases (exceptional but probable situations)

Thus, exposing your assumptions about the inputs and/or state of your application with assertions let the next developper understand a little more the purpose of your code.

A static analyzer (e.g. the compiler) might be happier too.

Finally, assertions can be removed from the deployed application using a single switch. But generally speaking, don't expect to improve efficiency with that: assertions checks at runtime are negligible.

1

As far as I know different programmers prefer one solution or the other.

The first solution is usually preferred because it is more concise, in particular, you do not have to check the same condition again and again in different functions.

I find the second solution, e.g.

public void registerPerson(Person person){
    if(person == null) throw new IllegalArgumentException("Input person is null.");
    persons.add(person);
    notifyRegisterObservers(person); // sends the person object to all kinds of objects.
}

more solid, because

  1. It catches the error as soon as possible, i.e. when registerPerson() is called, not when a null pointer exception is thrown somewhere down the call stack. Debugging becomes much easier: we all know how far an invalid value can travel through the code before it manifests itself as a bug.
  2. It decreases coupling between functions: registerPerson() does not make any assumptions about which other functions will end up using the person argument and how they will use it: the decision that null is an error is taken and implemented locally.

So, especially if the code is rather complex, I tend to prefer this second approach.

  • 1
    Giving the reason ("Input person is null.") also helps to understand the problem, unlike when some obscure method in a library fails. – Florian F Sep 23 '14 at 16:07
1

In general, yes, it is a good idea to "fail early". However, in your specific example, the explicit IllegalArgumentException does not provide a significant improvement over a NullReferenceException - because both objects operated upon are delivered as arguments to the function already.

But lets look at a slightly different example.

class PersonCalculator {
    PersonCalculator(Person p) {
        if (p == null) throw new ArgumentNullException("p");
        _p = p;
    }

    void Calculate() {
        // Do something to p
    }
}

If there was no argument checking in the constructor, you would get a NullReferenceException when calling Calculate.

But the broken piece of code was neither the Calculate function, nor the consumer of the Calculate function. The broken piece of code was the code that tries to construct the PersonCalculator with a null Person - so that is where we want the exception to occur.

If we remove that explicit argument check, you will have to figure out why a NullReferenceException occurred when the Calculate was called. And tracking down why the object was constructed with a null person can get tricky, especially if the code constructing the calculator is not close to the code that actually calls the Calculate function.

0

Not in the examples you give.

As you say, throwing explicitly isn't gaining you much when you're going to get an exception shortly thereafter. Many will argue that having the explicit exception with a good message is better, though I don't agree. In pre-released scenarios, the stack trace is good enough. In post-release scenarios, the call site often can provide better messages than inside the function.

The second form is providing too much info to the function. That function shouldn't necessarily know that the other functions will throw on null input. Even if they do throw on null input now, it becomes very troublesome to refactor should that stop being the case since the null check is spread throughout the code.

But in general, you should throw early once you determine that something has gone wrong (while respecting DRY). These though are maybe not great examples of that.

-3

In your example function, I would prefer you did no checks and just allow the NullReferenceException to happen.

For one, it makes no sense to be passing a null there anyway, so I'll figure out the problem immediately based on the throwing of a NullReferenceException.

Two, is that if every function is throwing slightly different exceptions based on what kind of obviously wrong inputs have been provided, then pretty soon you've got functions that might throw 18 different types of exceptions, and pretty soon you find yourself saying that's too much work to deal with, and just suppressing all exceptions anyway.

There's nothing you can really do to help the situation of a design-time error in your function, so just let the failure happen without changing it.

  • I have been working for a few years on a code basis that is based on this principle: pointers are simply de-referenced when needed and almost never checked against NULL and handled explicitly. Debugging this code has always been very time-consuming and troublesome since exceptions (or core dumps) occur much much later wrt the point where the logical error is. I wasted literally weeks looking for certain bugs. For these reason, I prefer the fail-as-soon-as-possible style, at least for complex code where it is not immediately obvious where a null-pointer exception comes from. – Giorgio Sep 15 '14 at 6:45
  • "For one, it makes no sense to be passing a null there anyway, so I'll figure out the problem immediately based on the throwing of a NullReferenceException.": Not necessarily, as Prog's second example illustrates. – Giorgio Sep 15 '14 at 7:04
  • "Two, is that if every function is throwing slightly different exceptions based on what kind of obviously wrong inputs have been provided, then pretty soon you've got functions that might throw 18 different types of exceptions...": In this case you can use the same exception (even NullPointerException) in all functions: what is important is to throw early, not to throw a different exception from each function. – Giorgio Sep 15 '14 at 7:05
  • That is what RuntimeExceptions are for. Any condition that "shouldn't happen" but prevents your code from working should throw one. You don't need to declare them in the method signature. But you need to catch them at some point and report that the function the requestd action failed. – Florian F Sep 15 '14 at 10:25
  • 1
    The question does not specify a language, so relying on e.g. RuntimeException is not necessarily a valid assumption. – user22815 Sep 15 '14 at 21:30

Not the answer you're looking for? Browse other questions tagged or ask your own question.