Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPP_NO_CHAR_SB_CTOR conflicts with PMD performance rule. #397

Open
linusjf opened this issue Jun 5, 2020 · 5 comments
Open

SPP_NO_CHAR_SB_CTOR conflicts with PMD performance rule. #397

linusjf opened this issue Jun 5, 2020 · 5 comments

Comments

@linusjf
Copy link

linusjf commented Jun 5, 2020

The rule
SPP_NO_CHAR_SB_CTOR from fb-
contrib

"This method constructs a StringBuffer or a StringBuilder using the constructor that takes an integer, but appears to pass a character instead. It is probable that the author assumed that the character would be appended to the StringBuffer/Builder, but instead the integer value of the character is used as an initial size for the buffer."

conflicts with the following PMD performance rule:
InsufficientStringBufferDeclaration

'Failing to pre-size a StringBuffer or StringBuilder properly could cause it to re-size many times during runtime. This rule attempts to determine the total number the characters that are actually passed into StringBuffer.append(), but represents a best guess "worst case" scenario. An empty StringBuffer/StringBuilder constructor initializes the object to 16 characters. This default is assumed if the length of the constructor can not be determined.'

The rule
SPP_NO_CHAR_SB_CTOR can be dropped.

I see no reason for this rule to exist which is evidently only catching a typo on the part of the user.

From what I understand, this rule intends to prevent a programmer from initialising a String[Builder|Buffer] with a size equal to the character specified in single quotes when the programmer intended to initialise the object with size 1 containing the said character.

The correct way is to append the char to the object.
I don't see why an integer value should be flagged, though. Only a character in quotes ought to be flagged.

There exists another PMD rule StringBufferInstantiationWithChar which disallows initialization of StringBuffer/StringBuilder with a char.

StringBuffer  sb1 = new StringBuffer('c');
StringBuilder sb2 = new StringBuilder('c');

// Disallow above two

StringBuffer good = new StringBuffer(41); // allow
good.append("This is a long string, which is pre-sized");

@linusjf
Copy link
Author

linusjf commented Jun 9, 2020

I understand that this bug falls under the category of SillyPotPourri of bugs in fb-contrib.

As someone who uses other static code analyzers such as Checkstyle and PMD in conjunction with Spotbugs, having to suppress this conflict every time is more than a minor irritant.
Secondly, there appears to be no way to specifically omit the bug from the suite of bugs in the PotPourri detector which appears to look for other silly mishaps as well.
I would still recommend dropping this one completely or reworking it as suggested above in the pmd examples.

@ThrawnCA
Copy link
Contributor

I'm unclear on why you think this is a false positive. Can you provide a sample of code that triggers the detector but should not?

@linusjf
Copy link
Author

linusjf commented Jun 11, 2020

It's not a false positive.
I don't think the rule is needed.
If the integer parameter is used to size the StringBuffer/StringBuilder as intended, it is definitely not a silly potpourri error anymore. The choice is quite deliberate and not a misinterpretation or misunderstanding of the constructor parameter. Suppressing the rule would work as well but it would have to be at the bug pattern level.
Are you indicating that you find this raised issue frivolous and not worthy of your attention or time?
You may close this issue if you intend to not pursue it further.

@ThrawnCA
Copy link
Contributor

ThrawnCA commented Jun 14, 2020

The point of the rule is to catch situations like:

StringBuilder s = new StringBuilder('a');

Presumably the developer intended to create a new StringBuilder with the default capacity, containing a single letter 'a'. But instead, what they will get is an empty StringBuilder with capacity 97 (the numeric value of 'a'). They should instead use new StringBuilder("a") or new StringBuilder().append('a').

If the rule fires on actual integers, then that would be a bug. Can you provide a code sample where that occurs?

@linusjf
Copy link
Author

linusjf commented Jun 15, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants