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.

user152468
  • 3,202
  • 6
  • 27
  • 57
Petro Semeniuk
  • 6,970
  • 10
  • 42
  • 65
  • 1
    In addition to your example with `checkArgument(T ref, boolean expr)`, recently there was an [issue #1038](https://code.google.com/p/guava-libraries/issues/detail?id=1038) discussing `Preconditions.checkArgument(T ref, Predicate test)` which was rejected. You can always create `Preconditions2` class with your own methods. – Grzegorz Rożniecki Jul 30 '12 at 07:30

3 Answers3

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.

Louis Wasserman
  • 191,574
  • 25
  • 345
  • 413
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.

JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • 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 '12 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 '12 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. – Sean Patrick Floyd Jul 29 '12 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 '21 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:

keyoxy
  • 4,423
  • 2
  • 21
  • 18