The smells (from Sonar) about hiding field names with variables actually expose that our naming scheme is a little messy. We often have this problem where we have a ValueExpression operand called something and then do this:
final ImmutableList<Optional<Value>> something = this.something.eval(parseState, encoding);
Which then hides the field. I understand why because after the eval()-call it's more or less the new version of the same value that we use from then on. But it may still be error-prone if we ever somehow want to use the field again (e.g., for another different evaluation).
Additionally, we use different naming schemes in some locations, like calling the variable somethingValues or evaluatedSomething.
In addition to this, there is a distinction between ValueExpressions where we are capable of processing a list of evaluated values and where we explicitly forbid returning a list and only accept a single, non-empty value (e.g., the size argument on Def). Still, we tend to name both types using singular nouns instead of plurals. With the singular values, we often also extract the final value, often as an int or BigInteger.
My proposal is to unify everything using the following style (example is from Expand, which has a plural argument (base) and a singular one (count)):
- Name constructor arguments and fields according to whether they are plural or singular. So the arguments and fields would be called
bases and count.
2.When evaluating the fields, prefix the names with evaluated, so in this case evaluatedBases and evaluatedCount. It's probably better to name these things -list instead of evaluated-. So it would then be baseList and countList.
- When passing the full list around to other methods, the argument names become suffixed with
Values to in this case baseValues. There seems to be no reason to pass evaluatedCount around as a list (see next item). Not sure whether we should keep this or keep using the list-suffix.
- When extracting one actual value from the list, use the singular version of the noun (because it always represents a single value) and suffix the names with
Value, so in this case baseValue (for example to bind a variable when iterating the list) and countValue.
The only question that remains is what we do with the many instances of the names operand, left and right? In many cases with basic operators there are no other names for the operands. But should we rename the cases where lists are acceptable then to use operands, lefts and rights? It somehow feels a bit weird, but it is the direct consequence of this naming scheme.
Originally posted by @jvdb in #273 (comment)
As discussed in #275, there are some problems with this proposal and we've decided:
- These names resolve the shadowing issues but they are far from perfect or desirable.
- Much more desirable are meaningful names, such as
multipliers (instead of left) and multiplicands (instead of right) for the operands of Mul.
- When meaningful names are not available, such as when writing highly generic code (e.g., implementation of
BinaryValueExpression) then we use generic names such as 'left', 'right' and 'operand'.
- These generic names are never plural, since they refer to what it is (a singular operand) and not what it represents (which can be plural, in case a list is acceptable as a result of evaluating the operand).
- When there are shadowing issues it is a good idea to use a standard scheme such as proposed at the top of this description.
All in all I would say we prefer a bit more pragmatic approach to naming things.
The smells (from Sonar) about hiding field names with variables actually expose that our naming scheme is a little messy. We often have this problem where we have a
ValueExpressionoperand calledsomethingand then do this:final ImmutableList<Optional<Value>> something = this.something.eval(parseState, encoding);Which then hides the field. I understand why because after the
eval()-call it's more or less the new version of the same value that we use from then on. But it may still be error-prone if we ever somehow want to use the field again (e.g., for another different evaluation).Additionally, we use different naming schemes in some locations, like calling the variable
somethingValuesorevaluatedSomething.In addition to this, there is a distinction between
ValueExpressions where we are capable of processing a list of evaluated values and where we explicitly forbid returning a list and only accept a single, non-empty value (e.g., thesizeargument onDef). Still, we tend to name both types using singular nouns instead of plurals. With the singular values, we often also extract the final value, often as anintorBigInteger.My proposal is to unify everything using the following style (example is from
Expand, which has a plural argument (base) and a singular one (count)):basesandcount.2.
When evaluating the fields, prefix the names withIt's probably better to name these things -evaluated, so in this caseevaluatedBasesandevaluatedCount.listinstead ofevaluated-. So it would then bebaseListandcountList.Valuesto in this casebaseValues. There seems to be no reason to passevaluatedCountaround as a list (see next item). Not sure whether we should keep this or keep using thelist-suffix.Value, so in this casebaseValue(for example to bind a variable when iterating the list) andcountValue.The only question that remains is what we do with the many instances of the names
operand,leftandright? In many cases with basic operators there are no other names for the operands. But should we rename the cases where lists are acceptable then to useoperands,leftsandrights? It somehow feels a bit weird, but it is the direct consequence of this naming scheme.Originally posted by @jvdb in #273 (comment)
As discussed in #275, there are some problems with this proposal and we've decided:
multipliers(instead ofleft) andmultiplicands(instead ofright) for the operands ofMul.BinaryValueExpression) then we use generic names such as 'left', 'right' and 'operand'.All in all I would say we prefer a bit more pragmatic approach to naming things.