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");
...
}
