So here's an easy solution: our own checkNotNull that throws InvalidArgumentException with a good error message. The error message can then be templated to allow terse invocations with just the method and parameter.
Unsure why I didn't go for this straightaway. On Fri, Jul 28, 2017 at 11:43 AM, Reuven Lax <[email protected]> wrote: > 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"); > > > ... > > > } > > > > > >
