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

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:

<pre lang="java">
if( parameter == null ) {
  throw new IllegalArgumentException( "parameter must not be null." );
}
</pre>

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:

<pre lang="java">
// 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" );
</pre>

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:

<pre lang="java">
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" );
</pre>

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.

<script src="https://gist.github.com/hstaudacher/5883964.js"></script>

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:

<pre lang="java">
public static <T extends Throwable> void doThrow( T type, String message, Object ... values ) throws T {
  ...
}
</pre>

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 Comments
  • Ralf Zahn
    Reply
    Posted at 6:09 pm, July 1, 2013

    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.

  • Gunnar
    Reply
    Posted at 6:31 pm, July 1, 2013

    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

  • Robert Konigsberg
    Reply
    Posted at 5:47 am, July 2, 2013

    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.

  • IBBoard
    Reply
    Posted at 10:08 am, July 2, 2013

    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]”.

  • Colin Decker
    Reply
    Posted at 4:57 pm, July 2, 2013

    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.

  • Robert Konigsberg
    Reply
    Posted at 5:08 pm, July 2, 2013

    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.

  • Greg Brown
    Reply
    Posted at 5:08 pm, July 2, 2013

    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);

  • Boxhead
    Reply
    Posted at 12:59 am, July 3, 2013

    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.

  • Bananeweizen
    Reply
    Posted at 1:30 pm, July 3, 2013

    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.

  • Posted at 1:46 pm, July 3, 2013

    @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.

  • Bananeweizen
    Reply
    Posted at 9:46 am, July 5, 2013

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

  • Jan
    Reply
    Posted at 7:37 pm, July 7, 2013

    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…

Post a Comment

Comment
Name
Email
Website