6

I really love how guava library allows simple one-liners for checking for null:

public void methodWithNullCheck(String couldBeNull) {
    String definitelyNotNull = checkNotNull(couldBeNull);
    //...
}

sadly, for simple argument check you need at least two lines of code:

public void methodWithArgCheck(String couldBeEmpty) {
    checkArgument(!couldBeEmpty.isEmpty());
    String definitelyNotEmpty = couldBeEmpty;
    //...
}

however it is possible to add method which could do argument check and return a value if check successful. Below is an example of check and how it could be implemented:

public void methodWithEnhancedArgCheck(String couldBeEmpty) {
    String definitelyNotEmpty = EnhancedPreconditions.checkArgument(couldBeEmpty, !couldBeEmpty.isEmpty());
    //...
}

static class EnhancedPreconditions {
    public static <T> T checkArgument(T reference, boolean expression) {
        if (!expression) {
            throw new IllegalArgumentException();
        }

        return reference;
    }
}

I just was wondering is that by design and if it is worth to put feature request for that.

EDIT: @Nizet, yeah, checks in methods could be clumsy. However checks in constructors for nulls looks really good and saves a lot of time spent on debugging NPEs:

public class SomeClassWithDependency {

    private final SomeDependency someDependency;

    public SomeClassWithDependency(SomeDependency someDependency) {
        this.someDependency = checkNotNull(someDependency);
    }

    //...

EDIT: Accepting Nizet's answer because I agree with him on side-effects and consistency reasoning. Also if you take a look into Xaerxess comment it looks like that causing confusion amongst other developers as well.

1
  • 1
    In addition to your example with checkArgument(T ref, boolean expr), recently there was an issue #1038 discussing Preconditions.checkArgument(T ref, Predicate<T> test) which was rejected. You can always create Preconditions2 class with your own methods. Jul 30, 2012 at 7:30

3 Answers 3

24

The biggest single reason that checkNotNull returns its argument is so it can be used in constructors, like so:

public Foo(Bar bar) {
  this.bar = checkNotNull(bar);
}

But the main reason that checkArgument doesn't do something similar is that you'd have to pass the argument separately anyway, and it just doesn't seem worth it -- especially with more complicated precondition checks, which can sometimes be more readable on their own line. Just because something can be a one-liner doesn't mean that it should be, if it doesn't increase readability.

1
  • 2
    I didn't know it did return the parameter... doing a "Code Golf" refactor :-)
    – fommil
    Jul 31, 2012 at 11:12
2

What I've never understood is why checkNotNull() returns its argument in the first place:

public void foo(String bar) {
    Preconditions.checkNotNull(bar);
    // here, you're sure that bar is not null. 
    // No need to use another variable or to reassign bar to the result 
    // of checkNotNull()
}

I personally ignore the result of checkNotNull(), as above. And this makes things consistent with the other checks which return void.

The only advantage I see is that you can do something like that, but I find it less readable than doing it in two separate lines:

public String trim(String bar) {
    return Preconditions.checkNotNull(bar).trim();
}

So, in short, I agree with you that the API is somewhat inconsistent, but I would prefer for all the methods to return void. A method should either have a side effect, or return something, but doing both should be avoided generally. Here, the goal of the method is to have a side effect: throwing an exception.

EDIT:

Your example is indeed a more valid explanation of why returning the argument is useful. But I would still have favored consistency and cleanness instead of this possibility of checking and assigning in a single line.

4
  • 5
    I would not do things like Preconditions.checkNotNull(bar).trim() instead of simple bar.trim(). In both cases you get the same NPE, so the test buys you nothing and you're only obscuring a simple thing. checkNotNull is intended for failing fast, so you avoid possibly incorrect partial execution of a method and get a shorter stack trace.
    – maaartinus
    Jul 29, 2012 at 16:50
  • If you write it as: return checkNotNull(bar, "bar may not be null").trim();, it starts making sense to do it. Anyway, I'm not saying it should be done. To the contrary: I think checkNotNull() should return void.
    – JB Nizet
    Jul 29, 2012 at 16:58
  • 1
    @JBNizet I respect the quality of your argumentation, which is why I won't downvote. In my opinion the checknotNull method is a very good choice for returning the variable and I wish there were more methods like that (e.g. someInt / Ints.checkNoneZero(someOtherInt). I think it's a nice way to highlight important code requirements without making methods unnecessarily long. I think it makes code more, not less readable. On the contrary, I don't see the point of using the method at all in your first example. Jul 29, 2012 at 22:47
  • I've got 1 usecase where checkNotNull() returns its argument. The call to superclass constructor: super(Preconditions.checkNotNull(param1))
    – adaslaw
    Apr 27, 2021 at 15:42
1

You can use valid4j with hamcrest-matchers instead (found on Maven Central as org.valid4j:valid4j)

For preconditions and postconditions:

import static org.valid4j.Assertive.*;

this.myField = require(argument, notNullValue());
this.myInteger = require(x, greaterThan(0));
...
return ensure(result, isValid());

For input validation:

import static org.valid4j.Validation.*;


validate(argument, isValid(), otherwiseThrowing(InvalidException.class));

Links:

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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