Comment From: pivotal-cla

@sigee Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Comment From: pivotal-cla

@sigee Thank you for signing the Contributor License Agreement!

Comment From: sbrannen

Looks good to me. What do you think @sbrannen ?

I think the extraction of getLowerBounds and getUpperBounds is a good improvement.

Regarding renaming all variables from lhs/rhs to leftHandSide/rightHandSide, I have mixed feelings about that: the lhs/rhs abbreviations are succinct and used in ClassUtils as well. So personally, I'd probably keep them as-is; however, the more explicit documentation for method parameters is certainly useful (even if we don't rename the parameters/variables).

If we do accept the renaming of those variables, the same should be applied in ClassUtils.

Comment From: sigee

Looks good to me. What do you think @sbrannen ?

I think the extraction of getLowerBounds and getUpperBounds is a good improvement.

Regarding renaming all variables from lhs/rhs to leftHandSide/rightHandSide, I have mixed feelings about that: the lhs/rhs abbreviations are succinct and used in ClassUtils as well. So personally, I'd probably keep them as-is; however, the more explicit documentation for method parameters is certainly useful (even if we don't rename the parameters/variables).

If we do accept the renaming of those variables, the same should be applied in ClassUtils.

@sbrannen, @rstoyanchev, If that helps, I can add one more commit to rename the lhs/rhs variables in ClassUtils as well.

Comment From: rstoyanchev

I have mixed feelings about that: the lhs/rhs abbreviations are succinct and used in ClassUtils as well

Agreed, the other argument is consistency, but considering the multitude of "leftHandSide" and "rightHandSide" variables involved, the result is less readable IMO. Possibly something shorter like target (lhs) and source (rhs), or leave as is.

Comment From: sigee

Please make the following changes.

  • revert changes to the parameter/variable names and keep the lhs and rhs prefixes (as they were) for brevity
  • update Javadoc for corresponding parameters to briefly explain what the lhs and rhs abbreviations mean -- something along the lines of @param lhsType the target type (left-hand side (LHS) type)
  • make similar changes to ClassUtils

@sbrannen Done

Comment From: sigee

sorry for not noticing that earlier

@sbrannen It is because I did the suggested renamings in the 2nd commit (5ad002d), but it was reverted since the lhs/rhs variables were reverted in the 4th commit (5374cc1).

Anyway, I did the suggested renamings in a new commit.

Comment From: sbrannen

This has been merged into 6.0.x and main.

Thanks