[java] UseObjectForClearerAPI Only For Public

[Vampire]

The description and implementation of the rule suggest that is should only apply
to public methods, but it is effective against all methods as for the existence
of the @Public attribute is tested instead of the value, so the condition is
always true.

With this fix, only public methods are considered like intended.

Fixes #1760

[pmd-test]

	1 Message
📖	This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 20 violations, 2 errors and 0 configuration errors. Full report
✅	This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 12 violations, 2 errors and 0 configuration errors. Full report

Generated by 🚫 Danger

[jsotuyod]

@Vampire thanks for the PR! You are absolutely right about this.

Would you care to include a unit test to make sure the issue is never reintroduced in the future? Just adding a new <test-code> under UseObjectForClearerAPI with a private method expecting 0 violations should be enough.

Thanks in advance!

[Vampire]

Sure, I'll have a look. One question though, is it intended that String and String[] are treated as the same?

[Vampire]

And if not, is it an error of the rule that is uses @Image, or is it an error of the AST, that the @Image for String[] is String?

[oowekyala]

@Vampire The ReferenceType node has an attribute @ArrayDepth to get the number of dimensions of the array.

Quite honestly I don't see the point of this method and why it only counts strings. There's already a rule TooManyParameters or ParameterCount idk whose proposed correction is to use an object. Maybe that one should be enriched with an option to ignore private methods instead, and this rule dropped. Is there something I'm not seeing about why string parameters in particular are bad?

[Vampire]

I don't know, I just wondered about why it complains about a private method when the description said it only checks public methods.
I guess this is also the reason it was in the "controversial" category before it was moved to the "design" category.

So independent of whether this gets deprecated and replaced by some other rule, when I'm at fixing this, should I also check the array depth as the description says it is only about Strings?

[Vampire]

I pushed a version now that has tests and also checks for array.

[adangel]

Let me chime in:

• The intention of the rule is clearly to only check for public methods. So, this bug is fixed by this PR. 👍
• The rule is very, very narrowed in its use case: only strings, it starts to complain at a fixed string parameter number of 3, so not configurable. I'd say, we should deprecate this rule and remove it with PMD 7. I'll put it onto the backlog in the wiki for now. -> https://github.com/pmd/pmd/wiki/PMD-7.0.0

[adangel]

Looks good, thanks!