I'm in favor of the wording in the style of the first: it's an immediately actionable message that will have an associated stack trace, but will provide the parameter in plaintext so the caller doesn't have to dig through the invoked code, they can just look at the documentation.
I've recently been convinced that all input validation should go through `checkArgument` (including for nulls) rather than 'checkNotNull', due to the type of exception thrown, so I'd usually prefer using that as the `Preconditions` method. Beyond that, +1 On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov < kirpic...@google.com.invalid> 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"); > ... > } >
