Yuck. I think that checkNotNull throwing a NPE is a very poor design choice from the author of checkNotNull.
On Fri, Jul 28, 2017 at 11:29 AM, Kenneth Knowles <[email protected]> wrote: > 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"); > > ... > > } > > >
