Standards for minimum/maximum properties

[vceratti]

Issue

The configurations for every rule is different and is confusing to understand all of them when there is no clear standard (#588). Also, individual PRs (#578) change the parameters name in minor versions, breaking our checks. In case of maximum values, the reporting works by checking value > maximum, while the minimum works with value <= minimum

I've made a list of maximum and minimum use cases for all the rules, according the docs:

• NPathComplexity minimum 200 The npath reporting threshold
• ExcessiveMethodLength minimum 100 The method size reporting threshold
• ExcessiveClassLength minimum 1000 The class size reporting threshold
• ExcessiveParameterList minimum 10 The parameter count reporting threshold
• ExcessivePublicCount minimum 45 The public item reporting threshold
• TooManyFields maxfields 15 The field count reporting threshold
• TooManyMethods maxmethods 25 The method count reporting threshold
• ExcessiveClassComplexity maximum 50 The maximum WMC tolerable for a class.
• NumberOfChildren minimum 15 Maximum number of acceptable child classes.
• DepthOfInheritance minimum 6 Maximum number of acceptable parent classes.
• CouplingBetweenObjects maximum 13 Maximum number of acceptable dependencies.
• ShortMethodName minimum 3 Minimum length for a method or function name
• ShortVariable minimum 3 Minimum length for a variable, property or parameter name.
• LongVariable maximum 20 The variable length reporting threshold

Proposal

In a next major release, make a standard along all the rules. Ideas:

1. Use threshold for all rules, reporting only when value < threshold or value > threshold. Pros.: standard for all rules; Cons.: depends on the rule name and/or documentation to understand if value < threshold or value > threshold is applied

2. Use maximum for all rules, reporting exclusively value > maximum. Pros.: standard for all rules and very clear criteria for reporting; works for almost all rules, as they usually define a maximum accepted limit. Cons.: don't work for ShortMethodName and ShortVariable (in those cases, a minimum accepted is necessary; could also merge those rules in a single one with both options (e.g. minimum => VariableName < maximum)

[kylekatarnls]

I approve your proposal 2. and it seems perfectly fine to me to keep minimum naming for ShortMethodName, ShortVariable

change the parameters name in minor versions, breaking our checks

We're aware of this, that's why we noticed it in the release notes of 2.7.0:

Please, also take note of a backwards incompatible property renaming in the CouplingBetweenObjects rule.

Note that adding a rule may also break your check. In fact if we want to guarantee that check would still pass after an update would be force users to white-list rules rather having by default all rules because any non-optional change can change the PHPMD output.

We won't do that, as the goal of PHPMD is to help developers to find possible improvements. It's up to the user to decide if a PHPMD check is mandatory, but it's still the user concern, we don't say a rule break is an emergency you should immediately fix and should stop everything else. PHPMD gives you hints about code smells and some kind of code health status. It does not aim to be blocking in your release or build process. So we fairly allow reasonable changes such as fixing a bad property name in minor releases.

[tvbeek]

@vceratti thanks for pointing out, I think it is a good idea to streamline this in the next major version.
My personal vote is for 2.

[prytoegrian]

My preference also goes to 2.

[ravage84]

@kylekatarnls well said, I totally agree with your statements. Yes, Let's go with the secodn proposal for PHPMD 3.0.0.

@vceratti we haven't really started worked on PHPMD 3.0.0, though. Would you be interested to take a stab at this issue once we do?

[kylekatarnls]

Revisiting this, as I'm trying to imagine what the common base for Rule could be in v3.

I like the idea of VariableName (or some verbose LengthOfVariableName) that would check both minimum => VariableName <= maximum simultaneously.

Indeed it would optimize it wince we would scan variable name only once and check the size we already have under the hand for both min and max. Also 1 variable name can only break minimum or maximum, but not both, unless max < min, but in this case it's a broken config: we could even throw an error for that before scanning. So it would fine to raise the first failing.

So now I'm also thinking how to make exceptions more consistent across rules and a bit more flexible. For instance allowing wildcard, i.e. test* would mean name starting with "test" never raise the error. And a sneaky case could be that one might want to add an exception _* that would not raise for minimum (so it allows _1, _x etc.) but not for maximum (raises for _aVeryVeryVeryVeryVeryVeryLongName).

In such case, it would be a bit convoluted to configure the rule, for instance you'd have to declare the rule twice with difference settings:

LengthOfVariableName:
  minimum: 4
  maximum: 999 # Huge number so not to check max
  exceptions: _*
  
LengthOfVariableName:
  minimum: 0  # Huge number so not to check min
  maximum: 20
  exceptions: test* # And so it can have different exception or no exception at all

WDYT? Are we still fine with merging ShortX and LongX rules even if it's a bit less convenient for some edge-cases to set exceptions?