When should an IllegalArgumentException be thrown?

Question

I'm worried that this is a runtime exception so it should probably be used sparingly.
Standard use case:

void setPercentage(int pct) {
    if( pct < 0 || pct > 100) {
         throw new IllegalArgumentException("bad percent");
     }
}

But that seems like it would force the following design:

public void computeScore() throws MyPackageException {
      try {
          setPercentage(userInputPercent);
      }
      catch(IllegalArgumentException exc){
           throw new MyPackageException(exc);
      }
 }

To get it back to being a checked exception.

Okay, but let's go with that. If you give bad input, you get a runtime error. So firstly that's actually a fairly difficult policy to implement uniformly, because you could have to do the very opposite conversion:

public void scanEmail(String emailStr, InputStream mime) {
    try {
        EmailAddress parsedAddress = EmailUtil.parse(emailStr);
    }
    catch(ParseException exc){
        throw new IllegalArgumentException("bad email", exc);
    }
}

And worse - while checking 0 <= pct && pct <= 100 the client code could be expected to do statically, this is not so for more advanced data such as an email address, or worse, something that has to be checked against a database, therefore in general client code cannot pre-validate.

So basically what I'm saying is I don't see a meaningful consistent policy for the use of IllegalArgumentException. It seems it should not be used and we should stick to our own checked exceptions. What is a good use case to throw this?

Solution 1

Treat IllegalArgumentException as a preconditions check, and consider the design principle: A public method should both know and publicly document its own preconditions.

I would agree this example is correct:

void setPercentage(int pct) {
    if( pct < 0 || pct > 100) {
         throw new IllegalArgumentException("bad percent");
     }
}

If EmailUtil is opaque, meaning there's some reason the preconditions cannot be described to the end-user, then a checked exception is correct. The second version, corrected for this design:

import com.someoneelse.EmailUtil;

public void scanEmail(String emailStr, InputStream mime) throws ParseException {
    EmailAddress parsedAddress = EmailUtil.parseAddress(emailStr);
}

If EmailUtil is transparent, for instance maybe it's a private method owned by the class under question, IllegalArgumentException is correct if and only if its preconditions can be described in the function documentation. This is a correct version as well:

/** @param String email An email with an address in the form [email protected]
 * with no nested comments, periods or other nonsense.
 */
public String scanEmail(String email)
  if (!addressIsProperlyFormatted(email)) {
      throw new IllegalArgumentException("invalid address");
  }
  return parseEmail(emailAddr);
}
private String parseEmail(String emailS) {
  // Assumes email is valid
  boolean parsesJustFine = true;
  // Parse logic
  if (!parsesJustFine) {
    // As a private method it is an internal error if address is improperly
    // formatted. This is an internal error to the class implementation.
    throw new AssertError("Internal error");
  }
}

This design could go either way.

• If preconditions are expensive to describe, or if the class is intended to be used by clients who don't know whether their emails are valid, then use ParseException. The top level method here is named scanEmail which hints the end user intends to send unstudied email through so this is likely correct.
• If preconditions can be described in function documentation, and the class does not intent for invalid input and therefore programmer error is indicated, use IllegalArgumentException. Although not "checked" the "check" moves to the Javadoc documenting the function, which the client is expected to adhere to. IllegalArgumentException where the client can't tell their argument is illegal beforehand is wrong.

A note on IllegalStateException: This means "this object's internal state (private instance variables) is not able to perform this action." The end user cannot see private state so loosely speaking it takes precedence over IllegalArgumentException in the case where the client call has no way to know the object's state is inconsistent. I don't have a good explanation when it's preferred over checked exceptions, although things like initializing twice, or losing a database connection that isn't recovered, are examples.

Solution 2

The API doc for IllegalArgumentException:

Thrown to indicate that a method has been passed an illegal or inappropriate argument.

From looking at how it is used in the JDK libraries, I would say:

• It seems like a defensive measure to complain about obviously bad input before the input can get into the works and cause something to fail halfway through with a nonsensical error message.

• It's used for cases where it would be too annoying to throw a checked exception (although it makes an appearance in the java.lang.reflect code, where concern about ridiculous levels of checked-exception-throwing is not otherwise apparent).

I would use IllegalArgumentException to do last ditch defensive argument checking for common utilities (trying to stay consistent with the JDK usage). Or where the expectation is that a bad argument is a programmer error, similar to an NullPointerException. I wouldn't use it to implement validation in business code. I certainly wouldn't use it for the email example.

Solution 3

When talking about "bad input", you should consider where the input is coming from.

Is the input entered by a user or another external system you don't control, you should expect the input to be invalid, and always validate it. It's perfectly ok to throw a checked exception in this case. Your application should 'recover' from this exception by providing an error message to the user.

If the input originates from your own system, e.g. your database, or some other parts of your application, you should be able to rely on it to be valid (it should have been validated before it got there). In this case it's perfectly ok to throw an unchecked exception like an IllegalArgumentException, which should not be caught (in general you should never catch unchecked exceptions). It is a programmer's error that the invalid value got there in the first place ;) You need to fix it.

Solution 4

Throwing runtime exceptions "sparingly" isn't really a good policy -- Effective Java recommends that you use checked exceptions when the caller can reasonably be expected to recover. (Programmer error is a specific example: if a particular case indicates programmer error, then you should throw an unchecked exception; you want the programmer to have a stack trace of where the logic problem occurred, not to try to handle it yourself.)

If there's no hope of recovery, then feel free to use unchecked exceptions; there's no point in catching them, so that's perfectly fine.

It's not 100% clear from your example which case this example is in your code, though.

Solution 5

As specified in oracle official tutorial , it states that:

If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception.

If I have an Application interacting with database using JDBC , And I have a method that takes the argument as the int item and double price. The price for corresponding item is read from database table. I simply multiply the total number of item purchased with the price value and return the result. Although I am always sure at my end(Application end) that price field value in the table could never be negative .But what if the price value comes out negative? It shows that there is a serious issue with the database side. Perhaps wrong price entry by the operator. This is the kind of issue that the other part of application calling that method can't anticipate and can't recover from it. It is a BUG in your database. So , and IllegalArguementException() should be thrown in this case which would state that the price can't be negative.
I hope that I have expressed my point clearly..

View more solutions

Comments

[djechlin]

I think "reasonably expected to recover" is weaselly. Any operation foo(data) could have happened as part of for(Data data : list) foo(data); in which the caller could want as many to succeed as possible even though some data are malformed. Includes programmatic errors too, if my application failing means a transaction won't go through that's probably better, if it means nuclear cooling goes offline that's bad.

[djechlin]

StackOverflowError and such are cases that the caller cannot reasonably be expected to recover from. But it sounds like any data or application logic level case should be checked. That means do your null pointer checks!

[Louis Wasserman]

In a nuclear cooling application, I'd rather fail hard in tests than allow a case the programmer thought was impossible to pass unnoticed.

[djechlin]

I think the advice "where the expectation is that a bad argument is a programmer error" is most consistent with how I've seen this used, so accepting this answer.

[Koray Tugay]

Why "you should never catch unchecked exceptions" ?

[Tom]

Because an unchecked exception is meant to be thrown as a result of a programming error. The caller of a method throwing such exceptions cannot reasonably be expected to recover from it, and therefore it typically makes no sense to catch them.

[svarog]

Because an unchecked exception is meant to be thrown as a result of a programming error helped me clear a lot of stuff in my head, thank you :)

[djechlin]

On the contrary, if an API client gives me a bad input, I should not crash my entire API server.

[Ignacio Soler Garcia]

Of course, it should not crash your API server but return an exception to the caller That shouldn't crash anything but the client.

[djechlin]

What you wrote in the comment is not what you wrote in the answer.

[Ignacio Soler Garcia]

Let me explain, if the call to the API with wrong parameters (a bug) is made by a third party client then the client should crash. If it is the API server the one with a bug calling the method with wrong parameters then the API server should crash. Check: en.wikipedia.org/wiki/Fail-fast

[djechlin]

I don't like this (oracle's) advice because exception handling is about how to recover, not whether to recover. For instance one malformed user request is not worth crashing an entire web server.

[Jeryl Cook]

Boolean.parseBoolean(..) , throws an IllegalArugmentException even though "the caller can reasonably be expected to recover." so.....its up to your code to handle it or fail back to the caller.

[Bart Hofland]

@djechlin . . . Sometimes it's not possible for the application to recover. That's often the case when illegal arguments are passed. It's something that the caller should fix instead of the called. That's where runtime exceptions come in. Regarding your example, I do agree that a malformed user request shouldn't crash an entire web server. But I do not consider a malformed user request to be an invalid argument in the first place. The web server should briefly analyze the invalid request and nicely throw that garbage back to the caller (possibly with hints about how to fix it). Problem solved.