Combining multiple input validations into one exception

Question

In order to make sure methods fail fast under invalid arguments, I usually do validation at the start of a method. If I'm verifying multiple predicates, this leads to a few lines of checkNotNull or checkArgument, where a NullPointerException or IllegalArgumentException is thrown if something isn't right.

public void takeoff(@NotNull Spaceship spaceship) {
 checkArgument(spaceship.fuel() > MIN_FUEL, "fuel too low");
 checkArgument(spaceship.pilot() != null, "pilot is missing");
 checkArgument(!spaceship.launchTower().isAttached(), "launch tower is still attached");
 // More validation, rest of function...
}
public static void checkArgument(boolean expression, String message) {
 if (!expression) {
 throw new IllegalArgumentException(message);
 }
}

This gives a neat block at the top of what assumptions are used in the rest of the method, which is useful for anyone else reading the method in the future. However, this is still a little inconvenient for the caller. If the fuel is too low, the pilot is missing, and the launch tower is still attached, the exception thrown is only IllegalArgumentException: fuel too low. Adding more fuel and re-running then gets the exception for the pilot; repeat for the tower and any other conditions that need to be met. The question: How can I group input validation so that all errors are given in the exception, while also keeping the code neat for reading?

Previously, I've done this pattern when bundling multiple exceptions into one, when the first exception thrown doesn't necessarily stop me from checking further:

public void takeoff(Spaceship spaceship) {
 IllegalArgumentException combined = null;
 try {
 checkArgument(spaceship.fuel() > MIN_FUEL, "fuel too low");
 } catch (IllegalArgumentException e) {
 combined = e;
 }
 try {
 checkArgument(spaceship.pilot() != null, "pilot is missing");
 } catch (IllegalArgumentException e) {
 if (combined != null) {
 combined.addSuppressed(e);
 } else {
 combined = e;
 }
 }
 
 try {
 checkArgument(!spaceship.launchTower().isAttached(), "launch tower is still attached");
 } catch (IllegalArgumentException e) {
 if (combined != null) {
 combined.addSuppressed(e);
 } else {
 combined = e;
 }
 }
 // repeat for remaining validations
 
 if (combined != null) throw combined;
 // rest of method
}

This adds all IllegalArgumentExceptions beyond the first as suppressed exceptions, but becomes way too verbose even at just three conditions to check, and a reader would have a harder time figuring out what those conditions even are. This was particularly relevant to me when using the javax.mail library, since methods like Message.setContent(), Message.setRecipients(), and Message.setSubject() could each throw a checked MessagingException. I very much wasn't expecting to encounter a situation where such an exception was thrown, but in the slim chance I did, I wanted to attempt all methods to collect as much information as possible about what's wrong before throwing the exception out of my method. Otherwise, the caller would need to run, see that something was thrown, fix the issue, run again, see another thing in the same area was wrong, fix, repeat.

The solution I'm looking for is something that can handle multiple calls throwing exceptions. While the spaceship validation could just be a few if-checks and then creating an error message for one IllegalArgumentException, the MessagingException situation requires exception handling. I'm primarily interested in solutions for Java, but I'd also like to see if other languages have tools for handling this problem as well.

• Greg Burghardt: In .NET, the AggregateException class might work for your needs. It is basically an exception representing a collection of exceptions. Maybe build something like that in Java?
• Doc Brown: Separate the validation and throwing of exceptions. Make checkArgument insert a message and/or an exception into some ArrayList of exceptions (instead of throwing), and when all validations happened, throw a collective exception in case the ArrayList isn't empty. That way, each validation by checkArgument is still a one-liner, not a full try-catch-if-else ceremony.
• Frank Hopkins: why would you say this is tedious for the caller? is this about debugging or handling the exception?
• Caleth: Failing fast has benefits of it's own, later checks can be moot if earlier checks fail. E.g. if spaceship is null, then none of the other validations are appropriate.
• EarthTurtle: @FrankHopkins This would be regarding debugging, throwing IllegalArgument at the first precondition that fails means there could be other issues with the arguments, but the caller wouldn't know until they do their fix and run it again. The goal is to fix any issues in the fewest number of runs/calls.

Answer 1

You’re using a hammer to swat a fly.

When you construct an exception you’re doing a lot.

You’re recording the line number where the problem occurred (and you’re recording the wrong one), building a stack trace (in your case the same one several times), and you’re choosing the name of the exception which decides where this will be handled (and again, you’re choosing this several times).

Do you really need to do that much?

So here’s my suggestion: since you insist on taking your sweet time with fail fast, stop thinking you have to throw the moment you detect a problem.

I believe there are many better alternatives. Let me attempt to illustrate one:

If the system is still stable enough to keep checking for problems then just record what you learned somewhere handy and keep checking.

Since you’re only going to recover in one place you only need one throw to take you there. And so you only need one exception object. Most exceptions allow you to cram your own string in them. Build one with your list of issues. When done checking throw if it’s not the empty string. You can even add new lines and make it readable.

I haven't tested this code, so don't trust it. But I think it could look like this:

public void takeoff(Spaceship spaceship) {
 String argumentErrors = "";
 String delimiter = System.lineSeparator();
 
 if (spaceship.fuel() < MIN_FUEL) {
 argumentErrors += "fuel too low" + delimiter;
 }
 if (spaceship.pilot() == null) {
 argumentErrors += "pilot is missing" + delimiter;
 }
 if (spaceship.launchTower().isAttached()) {
 argumentErrors += "launch tower is still attached" + delimiter;
 }
 // repeat for remaining validations 
 // (that you want sent to the same IllegalArgumentException handler)
 if (!argumentErrors.equals("")) {
 throw new IllegalArgumentException(argumentErrors);
 }
 // rest of method
}

I’m sure there are clever alternatives that improve on this but when I don’t need better I prefer good enough.

Also, this approach assumes that earlier problems won’t cause problems with later checks. That’s not true in every case so use with caution.

If you really had your heart set on having a checkArgument() method consider:

public void takeoff(@NotNull Spaceship spaceship) {
 String argumentErrors = "";
 
 argumentErrors += checkArgument(spaceship.fuel() <= MIN_FUEL, "fuel too low");
 argumentErrors += checkArgument(spaceship.pilot() == null, "pilot is missing");
 argumentErrors += checkArgument(spaceship.launchTower().isAttached(), "launch tower is still attached");
 // More validation...
 
 if (!argumentErrors.equals("")) {
 throw new IllegalArgumentException(argumentErrors);
 }
 // Rest of function...
}
public static String checkArgument(boolean expression, String message) {
 if (expression) {
 return message + System.lineSeparator();
 }
 return "";
}

BTW, if you really want to hide a negation in checkArgument() then it needs a better name to make the behavior obvious. Like say, require(). That way it will feel more like an assert than an if. I think it reads better:

public void takeoff(@NotNull Spaceship spaceship) {
 String argumentErrors = "";
 
 argumentErrors += require(spaceship.fuel() > MIN_FUEL, "fuel too low");
 argumentErrors += require(spaceship.pilot() != null, "pilot is missing");
 argumentErrors += require(!spaceship.launchTower().isAttached(), "launch tower is still attached");
 // More validation...
 
 if (!argumentErrors.equals("")) {
 throw new IllegalArgumentException(argumentErrors);
 }
 // Rest of function...
}
public static String require(boolean expression, String errorMessage) {
 if (expression) {
 return ""; // Got what we expected. No complaints.
 }
 return errorMessage + System.lineSeparator(); // Complain
}

There are many other ways to deal with errors. I’d get into it but I’ve already talked about them before.

If, however, you really feel the need for several exceptions then fine. Seems silly to me, how are you going to handle them all? But if you insist then for crying out loud stop throwing them every time. Exceptions are real, honest to god, objects. You don’t have to throw them the moment you construct them. Stick them in something. You can throw that something later.

Still though, I really think that’s more than you need.

Debugging is hard enough. Please be careful that you aren’t making it harder by being too clever.

It may be just because C++ template errors tortured me back in the day but I tend to distrust the second error message. I'd rather fix the first and rerun the test. Probably why I insist on fast tests.

---

I asked about it when mentioning the javax.mail library, since methods like setRecipient() and setSubject() have throws MessagingException in their signature. My assumption is that the methods could be independent, meaning that the fact that setRecipient() threw an exception does not mean that setSubject() is also guaranteed to fail. If someone provided bad args (like an invalid email address and a subject string that's too long) I'd want to communicate both errors, instead of exiting immediately with the MessagingException. - EarthTurtle

Setters are a different problem. Setters that throw exceptions typically hide their rules for throwing. I can see a few ways to deal with this. And I hate all of them.

• Accept the exceptions and just keep setting until they've all been thrown
• Dupe the rules outside of Message and check before it does
• Force Message to make it's rules public so they can be tested without using exceptions
• Pass everything in at once and make Message throw just one exception for all

Of the ones I hate I hate this last one the least. Problem is these last two both require Message to change. Which stinks when you don't own that code.

The dupe also stinks because you never know when Message will change and break your dupes. Single source of truth says no.

Which leaves us with your multiple exceptions. Sigh.

Ok Greg Burghardt had an idea here. The java version of AggregateException is Throwable.addSuppressed(Throwable). That gives us a handy place to dump a bunch of exceptions. But we still need readable code. What do you think of this:

ExceptionAggregator ea = new ExceptionAggregator();
ea.call( ()->message.setContent(content) );
ea.call( ()->message.setRecipients(recipients) );
ea.call( ()->message.setSubject(subject) );
if (ea.isThrown()) {
 throw ea.exceptions();
}

Maybe even...

if (ea.isThrown()) { 
 ea.setOuterException(new RuntimeException());// also calls addSuppressed() 
 throw ea.exceptions();
}

This would be the same as...

if (ea.isThrown()) {
 RuntimeException re = new RuntimeException();
 re.addSuppressed(ea.exceptions());
 throw re;
}

Maybe as simple as...

if (ea.isThrown()) {
 ea.throw(new RuntimeException()); // Does all that other stuff in one line
}

It should be noted that checked exceptions and lambdas haven't always gotten along. Some work arounds may be needed here. This looks like a promising one.

@FunctionalInterface
interface RunnableThrows<E extends Exception> {
 void run() throws E;
}

That seems to work around the problem. Now these lambdas can throw checked exceptions.

I feel like there's work to be done finding a way to log these each as a nice one liner but please forgive me if I leave that as an exercise for later.

• J_rite: Shouldn't all your checks be reversed? fuel < minfuel, spaceship.pilot == null and !attached -> attached?
• J_rite: Only in the part that you wrote with all the ifs, I don't know what the checkargument does
• candied_orange: @J_rite you are absolutely right. That's what I get for not testing the code. No idea why the OP has checkArgument negate the expression. If it had been named require or constraint. But check doesn't really tell me one way or another. Give me a moment...
• candied_orange: @J_rite better now?
• candied_orange: @J_rite not at all. Thanks for the code review! : )

Answer 2

I would be wondering: if there are two or three things that stop the method from succeeding, you fix the first one and then still have one or two reasons to fail, is that a problem? Do you gain anything of importance if you fix all problems together first time?

If not, then the current design would be perfectly fine. And fixing the first problem might fix others. Just make sure that if problem A causes problem B you report A first and not B.

• EarthTurtle: Wanting to fix all problems at once is desirable if it takes a long time to get to where the problem arises. If QA is testing and it takes 15m for the program to run before they get "fuel too low", then it will take another 15m to find the next problem. The assumption is that the problems are independent, where fixing one is not guaranteed to fix another, and the existence of one does not give information about the existence of another. And if the fix does not involve a code change, we'd want to communicate that too.
• Caleth: @EarthTurtle if it takes 15m to set up the environment for a test, work on making it take less time.
• gnasher729: @EarthTurtle In your scenario there would be a delay of 15 minutes from the first problem found to the exception being thrown. I’d say the fix for this problem needs to be elsewhere.
• EarthTurtle: @gnasher729 Oh I agree. The decision to devote time to that fix isn't up to me though :]

Answer 3

How can I group input validation so that all errors are given in the exception, while also keeping the code neat for reading?

Refactor the checkArgument method to return a List of strings without throwing exception and change the encapsulation by implementing the fluent interface design pattern so the domain specific language (DSL) changes to...

Check.argument(spaceship.fuel() > MIN_FUEL).message("fuel too low")
 .argument(spaceship.pilot() != null).message("pilot is missing")
 .argument(!spaceship.launchTower().isAttached()).message("launch tower is still attached")
 .test();

...with the two earlier mentioned changes implemented by...

import java.util.ArrayList;
import java.util.List;
public class Check {
 public static Argument argument(boolean condition) {
 return new Argument(condition);
 }
 public static class Argument {
 private boolean condition;
 private String message;
 private Argument previous;
 private Argument(boolean condition) {
 this.condition = condition;
 }
 public Or message(String message) {
 this.message = message;
 return new Or(this);
 }
 private List<String> test() {
 List<String> returnee = this.previous == null ? new ArrayList<>()
 : this.previous.test();
 if (this.condition == false) {
 returnee.add(this.message);
 }
 return returnee;
 }
 }
 public static class Or {
 private Argument argument;
 private Or(Argument argument) {
 this.argument = argument;
 }
 public Argument argument(boolean condition) {
 Argument returnee = new Argument(condition);
 returnee.previous = this.argument;
 return returnee;
 }
 public void test() throws IllegalArgumentException {
 List<String> messages = this.argument.test();
 if (messages.size() > 0) {
 IllegalArgumentException throwable = new IllegalArgumentException();
 for (String message : messages) {
 throwable.addSuppressed(new IllegalArgumentException(message));
 }
 throw throwable;
 }
 }
 }
}

Alternatively implement the java.util.function.Predicate interface that is a similar approach with a generic DSL.

Answer 4

It's really good that you're considering the Caller when thinking about Exceptions.
The most important part of Structured Exception Handling is that last bit - the Handling.
If you're expecting calling code to be able to recover from whatever Exception you throw, construct your Exceptions to support this. Custom Exception Types, custom properties detailing the problem, all that.

That said, one Exception per cause might be better, because it may not be the same [part of the] calling code that can Handle each one. Code very close to the called method might be able to deal with a FuelTooLow[Exception] but dealing with a TowerStillConnected[Exception] might have to go "further afield" in the call stack and a MissingPilot[Exception] even further still.
You definitely don't want the intervening code having to Catch-and-ReThrow anything, so sending separate Exceptions (messages?) for each failure case might be better. YMMV.