[java] The ClassNamingConventionsRule false-positive's on the class name "Constants"

[scottashipp]

Affects PMD Version:
6.29.0

Rule:

ClassNamingConventions

Please provide the rule name and a link to the rule documentation:

https://pmd.github.io/latest/pmd_rules_java_codestyle.html#classnamingconventions

Description:

The class name Constants triggers a P1 warning of the ClassNamingConventions rule. From the documentation, the regular expression should be [a-z0-9A-Z]* (notice the star which means "0 or more") but in the code, it uses [a-z0-9A-Z]+ (notice the plus, which means 1 or more).

See https://github.com/pmd/pmd/blob/master/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/ClassNamingConventionsRule.java#L33 where the "+" is defined.

See the documentation where it states:

This rule has the following properties:
...
classPattern [A-Z][a-zA-Z0-9]* Regex which applies to concrete class names

Because of the "+", it effectively requires that there be some prefix in the name. For example, FooConstants, or UhConstants would be valid but Constants is not.

Given the mismatch between the public documentation and the code, I believe this is unintentional.

Code Sample demonstrating the issue:

public class Constants {
   // ...
}

Expected outcome:

PMD reports a violation, but that's wrong. That's a false positive as far as I can see.

Running PMD through: Maven

[adangel]

Hm... your class Constants is obviously seen as a utility class instead of a "regular" class. Hence the pattern for utility classes is applied ([A-Z][a-zA-Z0-9]+(Utils?|Helper|Constants)).

I guess, you Constants class is indeed a utility class (e.g. has no non-static members).

Given the mismatch between the public documentation and the code, I believe this is unintentional.

This can't happen - this part of the documentation is generated from the code - so it always matches...

The question is more like - should we allow the very generic name "Constants" as a name of a utility class. The question the comes up is "What constants?" - Constants can be everything, so not sure if it is a useful name...

By the way: You can configure the rule yourself: Configuring rules. Just set the regex you want.

This issue would be about changing the default regex - and I'm not sure, we should do this.

[scottashipp]

@adangel TBH I don't agree with the question you have stated. That isn't my contention and I didn't file this to debate a question like that. If I did, I'd be willing to make the argument that now that we have modules in Java 9 and packages can better encapsulate related concepts, the name Constants for a class would actually make sense in some contexts considering that the package it is in already categorizes it.

But that's not my argument nor what I'm here for. The reason I filed the ticket is that this rule triggers a violation on a case it obviously wasn't intended to, which is clearly an unintentional side effect. I looked back at the history of the rule before filing. It is based on a tweet from Kevlin Henney about classes named _____Helper or ____Utils. I am not sure how or when "Constants" got lumped into this but there's certainly nothing about disallowing the class name Constants in the rule history or in the Java ecosystem in general. The closest you can say is that a large class of global constants (whatever it's name) is considered an anti-pattern. (Or, but this isn't that close, an interface that holds constants.)

But this is the class naming convention rule. There are other rules for those particular anti-patterns.

[scottashipp]

I have spent the time to track down when and how this came about and I think it definitively shows that this is an accidental outcome and should be changed.

In #1164 the definition of "utility class" was found to apply to classes containing only constants so the resulting PMD violation indicated that a constants class needed to be renamed to end to "Utils" or "Helper". In order to try and resolve this, someone submitted a PR to have it suggest "Constants" as well. The change was released with PMD 6.23.0.

It's clear that a class named only "Constants" was never considered at all, and the fact that this rule triggers a violation here is completely accidental.

If a rule has a false positive then I think we all agree it should be changed.

I'd like to propose a solution. At some point in the past, the ClassNamingConventions rule only enforced that class names start with capital letters. It feels like PMD has waded into a bizarre area by deciding that by default only "Utils" or "Helper" are allowed on classes deemed as "utility classes." Perhaps its time to restore the rule to its original meaning and only check that classes are Pascal case. This is certainly the only convention that we can safely say is shared by the vast majority of the Java ecosystem.

[adangel]

Thanks for taking time to look into this.

If a rule has a false positive then I think we all agree it should be changed.

Agreed.

Whether that's a false positive or not depends on what the rule should do. You are correct in questioning, whether this rule does more than it should.

ClassNamingConventions is one of the area, where endless bike-shed discussions are possible. Hence the rule is very configurable, because usually the default settings don't fit everybody's need.
E.g. - if you don't want to use the (probably not perfect) utility class detection, you can set this property to the same pattern like the class name pattern:

<rule ref="category/java/codestyle.xml/ClassNamingConventions">
    <properties>
        <property name="utilityClassPattern" value="[A-Z][a-zA-Z0-9]*" />
    </properties>
</rule>

This effectively allows "Constants" as a valid class name (Disclaimer: I didn't try this).

Please also note, that you are not the first person questioning this: e.g. #1595 #1096
There is also the request to increase the responsibility of this rule and add a testClassPattern: #2867

One way forward would be: Make the ClassNamingConventionsRule simple - don't care about utility classes or anything other special. Basically, what you are proposing.
A middle ground would be, to use the same pattern for utilityClasses as for "normal" classes by default - if someone wants to handle utilityClasses differently, then it would be possible to configure the rule. We still have to describe, what PMD thinks, a utility class is (#1102).
On the other end of the possibilities is: Leave this rule as is - you need to override the default configuration, if you don't want to use the utility detection.

I have no strong opinion on this. Whatever we decide, there will always be someone who is unhappy with the decision.

[adavis100]

+1 to changing the default for utility classes to be the same as other classes. As you say, whatever you do will make someone unhappy, but that seems like a sensible default that still allows customization.

Constants may not be the greatest name, but it does seem that it was unintentionally disallowed here.

I don’t think there's an agreed upon naming scheme for utility/helper classes (or even agreement as to whether they’re a good thing). This rule flags better named utility/factory classes such as the common convention of adding an ’s’ to the related class name, e.g., java.util.Collections.

[scottashipp]

On the other end of the possibilities is: Leave this rule as is - you need to override the default configuration, if you don't want to use the utility detection.

@adangel I appreciate that. I did reconfigure the regular expression for the current project I saw this violation on. Which is great for me. Perhaps it's not so great for others, though. With defaults, there's always a cost on the ecosystem in general. If it takes everyone an hour to figure out whether or not they want to accept the default prescription that all "utility classes" end in Helper, Util(s), or Constants, and then (if not) reconfigure the rule, then the cost could be thousands of hours considering the thousands of projects using PMD. Particularly for existing users, as the way this all came about for me was upgrading PMD from a prior version where the utility class naming feature was not part of the rule.

But it's probably worse than that because the default psychological response to a PMD violation that doesn't make immediate sense is something like "hmmm I guess I'll suppress it for now and figure it out later" which, if they suppress it globally, leads to losing the benefits of the rule altogether. And it's actually a very good rule.

So that's a long way of explaining why I spent even more time opening an issue and writing all these comments. If it potentially could save thousands of hours, it's worth it.

I do also appreciate @adavis100 's point about utility classes named similar to prominent examples in the Java standard library. I guess this rule would flag them as a violation also. In more recent versions of Java, the practice seems to have changed to include such utility methods as default methods in the related interface but there are certainly plenty of classes around named similarly to Collections...even Math is an example, I suspect.

I think I would vote for the "middle ground" option that @adangel proposed after further consideration.

[adangel]

Alright - I think the most sensible way forward is to keep the property "utilityClassPattern" (this is any needed for compatibility reasons), but change the default value to [A-Z][a-zA-Z0-9]*. That way, this feature is made an opt-in feature, and anyone who wants to verify utility naming classes can enable it back.

[scottashipp]

@adangel Thank you! This change helps a lot! It really makes a difference!