Hi,

I suggest we use requireNonNull(var, "var") pattern rather
than requireNonNull(var).

Then error message would include which variable/field turned out to be null.
It would make a big difference in case a constructor verifies multiple
parameters like in Correlate:

  protected Correlate(
      RelOptCluster cluster,
      RelTraitSet traitSet,
      RelNode left,
      RelNode right,
      CorrelationId correlationId,
      ImmutableBitSet requiredColumns,
      JoinRelType joinType) {
    super(cluster, traitSet, left, right);
    assert !joinType.generatesNullsOnLeft() : "Correlate has invalid join
type " + joinType;
    this.joinType = requireNonNull(joinType);
    this.correlationId = requireNonNull(correlationId);
    this.requiredColumns = requireNonNull(requiredColumns);

If a user makes a mistake and passes null to joinType or correlationId,
then Calcite would throw NPE, however, it would be hard to tell which
parameter was null.
Line numbers are known to drift over time.

I suggest we use the following pattern consistently:

    this.joinType = requireNonNull(joinType, "joinType");
    this.correlationId = requireNonNull(correlationId, "correlationId");
    this.requiredColumns = requireNonNull(requiredColumns,
"requiredColumns");

The PR is https://github.com/apache/calcite/pull/2293

The suggested change automatically aligns single-word message with the
first argument to requireNonNull.
In other words, "./gradlew style" command would insert/fix the messages
automatically.
I assume modern IDEs enable developers to configure var =>
requireNonNull(var, "var") expansion (I use it in IDEA).

Currently we have
545 usages of requireNonNull(var) <-- this is to be updated to (var, "var")
359 usages of requireNonNull(var, "var")
37 usages of requireNonNull(var, () -> ..)
33 usages of requireNonNull(var, "custom message")

If no-one objects within three days, I'll assume lazy consensus and commit
it.

Vladimir

Reply via email to