when( true ).throwIllegalArgument( “something went wrong” );

As a framework developer I have to make sure that my framework works properly – and among other things, that means it has to be used in the way that I meant it to be used. One option to achieve this is to validate the input. With input I basically mean method parameters. For example, a parameter is not allowed to be null or a list must not be empty and so on. When trying to follow this rule, you end up with code that looks like this:

if( parameter == null ) {
  throw new IllegalArgumentException( "parameter must not be null." );
}

As you can imagine, this gets really messy when you write this over and over. Luckily we all know the DRY principle and that leads us to putting the validation code into a utility class. But wait a minute. There must be a solution for a pattern that is so common, right? Right! As you may know from previous posts, I’m a big fan of Google Guava. Guava provides a nice little helper class called Preconditions. The methods of this class are used only for validation and I was really convinced that this was the way to go. The important part of the last sentence is “was”. During the last month I started to have some doubts. The first one is pretty minor but still valid. The name of this class is Preconditions. But I found myself often using the methods after I have written some functionality, so I was really using them as “PostConditions”. Alright, this is minor because you can use static imports and the “Preconditions” wording does not uglify your code. The second problem with Preconditions is the use of negative logic.

Let me explain this with an example. The Preconditions class has a method called checkArgument. This method takes a boolean parameter for validation. When this parameter is false an IllegalArgument exception is thrown. But, I think this should work exactly the other way around: when the parameter is true the exception should be thrown. A small snippet will show why:

// As it is
Preconditions.checkArgument( !list.isEmpty(), "List must not be empty" );
Preconditions.checkArgument( list != null, "List must not be null" );
Preconditions.checkArgument( array.length != 0, "Array must not be empty" );
// As it should be
Preconditions.checkArgument( list.isEmpty(), "List must not be empty" );
Preconditions.checkArgument( list == null, "List must not be null" );
Preconditions.checkArgument( array.length == 0, "Array must not be empty" );

As you can see, this example shows three calls to its Preconditions methods. When I read those methods it simply feels wrong because of the inverted logic. That is, I want an exception when the list is empty, but here I have to invert the isEmpty result. The good news is that I’m not alone with this opinion. When I talked about this problem with my colleagues they all agreed and said things like, “I always do it wrong the first time…”. So, long story short, we think we’ve created a cleaner solution.

You might be familiar with Mockito, but if not, read this. With mockito you write lines such as this: when( mock.getId() ).thenReturn( "id" );. Now, why not use this pattern for validation? Looking at the snippet again, it could be transformed to something like this:

when( list.isEmpty() ).throwIllegalArgument( "List must not be empty" );
when( list == null ).throwIllegalArgument( "List must not be null" );
when( array.length == 0 ).throwIllegalArgument( "Array must not be empty" );

Much better, right :)? We named the class that contains these methods Clauses because each use defines the condition of a contract such as “we agree that this list should not be empty”. I have created a basic implementation and published it together with some tests to give you the gist. You’ll see that the implementation is pretty straight forward.

Feel free to clone and modify this class as often as you’d like. I would love to see some contributions! With our Tabris framework I’ve been using this class since last week and it still feels right ;). Let’s see what time brings. There is one thing I wanted to add. A colleague of mine came up with an idea that goes one step further. He would like to see the following method within the Clauses class:

public static <T extends Throwable> void doThrow( T type, String message, Object ... values ) throws T {
  ...
}

I really like the generic approach. The only drawback is that it needs reflection to create the exception instances. On the other hand it would allow the use of individual exception types. I simply didn’t have the time to add this method. Feel free to do so if you’d like ;).

Thinking about the future of the Clauses class I would really like to see it in Guava. What do you think? Maybe a Guava contributor is reading this and will step up… ;). Meantime, kudos go to my colleagues Frank Appel and Moritz Post for their great input!

14 Responses to “when( true ).throwIllegalArgument( “something went wrong” );”

  1. Ralf Zahn says:

    IMHO it is not a good idea to create an instance of Clause for each when(…) invocation, because object creation leads to memory and performance leaks, esp. when used a lots of times.

    There’re only 2 states that Clause instances might have, so why not create those 2 instances (one for true and one for false) and cache them? This should not lead to multi-threading problems, so there isn’t any argument against this approach.

  2. Gunnar says:

    You might be interested in the method validation feature in Bean Validation 1.1 which makes this kind of parameter checks even simpler by just putting constraint annotations to the parameters:

    public void doSomething(@NotNull String name, @NotEmpty List items>) { … }

    When working with a container such as CDI or Spring, these constraints can be checked automatically upon method invocation without requiring any further manual coding.

    –Gunnar

  3. Robert Konigsberg says:

    Agreed, the object construction is by default a bad idea. Now, with garbage collectors as they are, we could be dealing with very efficient object construction, but I’d hesitate to rely on such a low-level library in a large project, at least, not without thorough and strong performance numbers to back it up.

  4. IBBoard says:

    Isn’t it just that the Guava preconditions are following the assert pattern? The reading of those is “Precondition is that [condition] must be true, otherwise throw an exception with [message]“, much like “Assert that [condition] must be true, otherwise thrown an assertion error with [message]“.

  5. Colin Decker says:

    I find it curious that you see it as “inverted logic” to express “list is not empty” as “!list.isEmpty()” but correct to express it as “list.isEmpty()”. As I see it, nothing could be further from the truth! One reads as “not list is empty”, the other as “list is empty”. Logically speaking, which expresses the condition that must be true better?

    Yes, using if-condition-throw to check, you’re forced to use negative logic. But Preconditions frees you from that arbitrary necessity and allows you to easily express a series of conditions that are guaranteed to be _true_ if the checks pass. In my opinion, it’s much easier for a reader of the code to understand the pre/postconditions you’re expressing when the expression itself is the condition that must be true and does not need to be inverted to understand the state.

  6. Robert Konigsberg says:

    Not my idea, but one that makes this idea more palatable:

    1. Don’t create an object that holds a single boolean. Create two objects in advance. For instance:

    public static Clause when( boolean condition ) {
    return condition : Clause.TRUE : Clause.FALSE;
    }

    Clause.FALSE = new Clause() {
    public void throwIllegalArgument( String message ) { }
    }

    Clause.TRUE = new Clause() {
    public void throwIllegalArgument( String message ) {
    throw new IllegalArgumentException( message );
    }
    }

    Replacing object creation with two function calls.

    At this point the VM can do a much better job optimizing.

  7. Greg Brown says:

    I don’t generally bother with the message part when validating arguments:

    if (foo == null) {
    throw new IllegalArgumentException();
    }

    It keeps the code simpler, and anyone who runs into the exception can examine the stack trace to see what went wrong.

    Of course, you can also use assert for this:

    assert(foo != null);

  8. Boxhead says:

    I agree with Gunnar bean validation (jsr303) is really the way to go, or as an alternative you could consider the Oval project http://oval.sourceforge.net/ which is a bit easier to set up and is more flexible. That said I like the concept behind your idea.

  9. Hi everyone and thanks for your great feedback.

    @Ralf, @Robert: I have updated the gist to use static references to plug the performance leak.

    @Greg: I don’t think leaving out error messages is an option for us because they contain useful information. Think about a method that expects an int > 0. When the user passes in 0 the exception without a message would be pretty useless. But if the message would say “Expected is an argument > 0 but was 0″ the user can fix the problem without any pain.

    @Colin: Yes I call it inverted logic because reading checkArgument( !list.isEmpty(), “message” ); means to me -> throw an IllegalArgument exception when the list is NOT empty. That simply feels wrong to me. I’m not alone with this opinion. I talked to several other developers and they fully agree. So, in the end it’s a question taste, right :)?

  10. Bananeweizen says:

    To my mind you are fixing the wrong problem. :) As seen by the discussion above, the aspect of reading this as inverted or not inverted logic is quite subjective. And the root cause for that is not that one of you is right or wrong, the reason is that the naming of the method (and its arguments) does not indicate what the method really does.

    Preconditions.checkArgument(criteria) might do anything with the criteria. One can assume the criteria to be the conditions for failure or for success. The method name does not really give a clue. Have a look at Junits assert instead. “assert” perfectly indicates that the condition must be fulfilled. There is no discussion possible. Now imagine the above method to be called either

    Preconditions.assertArgument(criteria)
    Preconditions.denyArgument(criteria)

    Everyone would understand perfectly what logic is used to evaluate the criteria.

  11. Moritz Post says:

    @Bananenweizen

    You are right about the readability of the checkArguement() statement. It doesn’t convey what happens in the method. That is why the post proposes another solution with a when().throw() syntax. You should reread that part again i think.

  12. Bananeweizen says:

    @Moritz: You are right. Seems like I skipped the last part of the post when reading this 2 days before. Thanks.

  13. Jan says:

    Spring contains the class org.springframework.util.Assert which does what you need. It goes even one step further by throwing an IllegalArgumentException by default.

    You can write these assertions (amongst others):
    Assert.notEmpty(list);
    Assert.isNull(object);
    Assert.notNull(object);
    Assert.hasText(“my Streing”);

    As you the Spring guys are framework developers and that’s why they’ve coded that class. That was 11 years ago…

  14. @Jan, thanks but as you already said, “That was 11 years ago..” and the API looks like this. I think the Clauses approach is a more modern way to achieve the same.

14 responses so far

Written by . Published in Categories: EclipseSource News, Editors choice, Planet Eclipse