As to the choice of check method: - checkArgument throws an InvalidArgumentException, which is clearly in "your fault" category, a la HTTP 4xx - checkNotNull throws an NPE, which is usually a "my fault" exception, a la HTTP 5xx.
The docs on NPE are not clear on blame, and this is a bug in the docs. But almost all the time, an NPE indicates that the line where it is thrown is incorrect. InvalidArgumentException is unambiguous. This could also be called a bug in checkNotNull. It throws the same exception as if you _forgot_ to check if it was not null. So it sort of doesn't do one of the most important things it should be doing. As to verbosity: All error messages should be actionable. We have a chronic problem with terrible or nonexistent error messages. NPE is uninformative and this feeds into the prior two bullets: If I see "NPE on line XYZ of file ABC" I am _always_ going to file a bug against the author of file ABC because they dereferenced null. Their fix might be to simply protect themselves with a checkArgument to clearly blame their caller, but that is a totally acceptable bug/fix pair. We should really get an analysis in place based on @Nullable annotations to mitigate this a bit, too. Kenn On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov < [email protected]> wrote: > Hey all, > > I think this has been discussed before on a JIRA issue but I can't find it, > so raising again on the mailing list. > > Various IO (and non-IO) transforms validate their builder parameters using > Preconditions.checkArgument/checkNotNull, and use different styles for > error messages. There are 2 major styles: > > 1) style such as: > checkNotNull(username, "username"), or checkArgument(username != null, > "username can not be null") or checkArgument(username != null, > "username must be set"); > checkArgument(batchSize > 0, "batchSize must be non-negative, but was: %s", > batchSize) > > 2) style such as: > checkArgument( > username != null, > "ConnectionConfiguration.create().withBasicCredentials( > username, > password) " > + "called with null username"); > checkArgument( > !username.isEmpty(), > "ConnectionConfiguration.create().withBasicCredentials( > username, > password) " > + "called with empty username"); > > Style 2 is recommended by the PTransform Style Guide > https://beam.apache.org/contribute/ptransform-style-guide/#transform- > configuration-errors > > However: > 1) The usage of these two styles is not consistent - both are used in about > the same amounts in Beam IOs. > 2) Style 2 seems unnecessarily verbose to me. The exception thrown from a > checkArgument or checkNotNull already includes the method being called into > the stack trace, so I don't think the message needs to include the method. > 3) Beam is not the first Java project to have validation of configuration > parameters of something or another, and I don't think I've seen something > as verbose as style 2 used anywhere else in my experience of writing Java. > > What do people think about changing the guidance in favor of style 1? > > Specifically change the following example: > > public Twiddle withMoo(int moo) { > checkArgument(moo >= 0 && moo < 100, > "Thumbs.Twiddle.withMoo() called with an invalid moo of %s. " > + "Valid values are 0 (inclusive) to 100 (exclusive)", > moo); > return toBuilder().setMoo(moo).build();} > > into the following: > > public Twiddle withMoo(int moo) { > checkArgument(moo >= 0 && moo < 100, > "Valid values for moo are 0 (inclusive) to 100 (exclusive), " > + "but was: %s", > moo); > return toBuilder().setMoo(moo).build();} > > > And in simpler cases such as non-null checks: > public Twiddle withUsername(String username) { > checkNotNull(username, "username"); > checkArgument(!username.isEmpty(), "username can not be empty"); > ... > } >
