Skip to content

[java] UseObjectForClearerAPI Only For Public#1752

Merged
adangel merged 2 commits into
pmd:masterfrom
Vampire:UseObjectForClearerAPI-only-for-public
Apr 5, 2019
Merged

[java] UseObjectForClearerAPI Only For Public#1752
adangel merged 2 commits into
pmd:masterfrom
Vampire:UseObjectForClearerAPI-only-for-public

Conversation

@Vampire

@Vampire Vampire commented Apr 2, 2019

Copy link
Copy Markdown
Contributor

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

@ghost

ghost commented Apr 2, 2019

Copy link
Copy Markdown
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

jsotuyod commented Apr 3, 2019

Copy link
Copy Markdown
Member

@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!

@jsotuyod jsotuyod added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Apr 3, 2019
@Vampire

Vampire commented Apr 3, 2019

Copy link
Copy Markdown
Contributor Author

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

@Vampire

Vampire commented Apr 3, 2019

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

@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

Vampire commented Apr 3, 2019

Copy link
Copy Markdown
Contributor Author

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 Vampire force-pushed the UseObjectForClearerAPI-only-for-public branch from f710edb to f3c0d39 Compare April 3, 2019 10:00
@Vampire

Vampire commented Apr 3, 2019

Copy link
Copy Markdown
Contributor Author

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

@adangel adangel self-assigned this Apr 5, 2019
@adangel

adangel commented Apr 5, 2019

Copy link
Copy Markdown
Member

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 adangel added a:false-positive PMD flags a piece of code that is not problematic and removed is:WIP For PRs that are not fully ready, or issues that are actively being tackled labels Apr 5, 2019
@adangel adangel added this to the 6.14.0 milestone Apr 5, 2019
@adangel adangel changed the title [java] Useobjectforclearerapi Only For Public [java] UseObjectForClearerAPI Only For Public Apr 5, 2019

@adangel adangel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@adangel adangel merged commit f3c0d39 into pmd:master Apr 5, 2019
adangel added a commit that referenced this pull request Apr 5, 2019
@Vampire Vampire deleted the UseObjectForClearerAPI-only-for-public branch April 5, 2019 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:false-positive PMD flags a piece of code that is not problematic

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[java] UseObjectForClearerAPI flags private methods

4 participants