Reduce unnecessary asserts

[damithc]

We need to define where to use asserts and where to omit them. For example, asserting every parameter to every method as not null may be unnecessary.

[pyokagan]

Official java guide on assertions: http://docs.oracle.com/javase/8/docs/technotes/guides/language/assert.html

In particular, I found this interesting:

Do not use assertions for argument checking in public methods.

Argument checking is typically part of the published specifications (or contract) of a method, and these specifications must be obeyed whether assertions are enabled or disabled. Another problem with using assertions for argument checking is that erroneous arguments should result in an appropriate runtime exception (such as IllegalArgumentException, IndexOutOfBoundsException, or NullPointerException). An assertion failure will not throw an appropriate exception.

which hints that we should not be doing assert someArgument != null; in our public methods, but rather throwing NullPointerException instead.

[damithc]

@pyokagan

Argument checking is typically part of the published specifications (or contract) of a method, and these specifications must be obeyed whether assertions are enabled or disabled.

In practice, are we able to do proper argument checking in all public methods? It's one thing to check arguments to a really public API such as a component API (e.g. Model, Storage) but argument checking in all public methods in classes inside a component can be messy. So in practice, we could end up not doing any argument checking at all in those classes which is worse than using asserts?.

[MightyCupcakes]

I've done some research on this and found out Google Guava's Preconditions

The nice thing about Preconditions is that it will throw the necessary exceptions like NullPointerException for null checks (instead of a very generic assertion failure) and its method names like checkNotNull makes it very clear what we are trying to do.

As an added bonus, checkNotNull (and any other methods in the Preconditions package) will return the validated value if it is not null so we can always do something like this

public void fooMethod(Foo args) {
    return bar(checkNotNull(args))
}

This ensures that the args passed to bar() is not null - if it is then a NullPointerException is thrown. This is compliant with the official java guide for assertions (that is to throw a null pointer for null values) without having to mess the code up too much.

[damithc]

Related PR oss-generic/process#38