Why are duplicated String literals so bad?

[Michael Piefel]

Hello,

I recently updated our quality profiles. In the process, S1192 was assigned severity Critical, which is the default from the Sonar Way. Since that moment, many projects have turned to be in a bad shape.

Now, of course duplicated literals are a possible source of bugs, but is this really critical? Looking at one project, I find:

• A class building some HTML. Naturally, there are some Strings like "</table>" in there. Will a constant like CLOSE_TABLE_TAG improve readability?
• Long SQL queries split across lines (because Java lacks multi-line Strings). Not surprisingly, some queries refer to the same columns, so they share fragments like ", a.partnerid as account_partnerid ". Defining a constant for that means that
• “Edit SQL fragment” in IntelliJ will not work anymore
• IntelliJ cannot detect that I refer to the wrong column
• I cannot immediately see what the column will be called to refer to in the following result set accesses
• Debug and warning messages. I may or may not change some of them to be more specific in the future. They may contain placeholders that I cannot immediately see if they are wrapped in a constant.

Since the code is actually quite clean and actually uses many String constants and enums, we have, all in all, about 95% of reported cases where changing would not improve anything, even do harm. It seems to be quite hard to filter out cases like the above automatically. I wonder whether a rule which so easily reports irrelevant places should really be enabled by default.

For the remaining cases, I wonder whether the default severity of Critical is justified. Nothing is really broken, after all. Is that more severe than possible null accesses?

What do you think?

Greetings,

   Michael

[Brian Sperlongano]

Hi,

I have found in general, that yes, these are bad because it's quite easy to change one constant, thinking you're changing that constant everywhere.  It's a maintainability nightmare!  However, everyone's code base and technology set are different, so if a finding isn't meaningful for your specific case you could always just deactivate that rule.

Thanks,

Brian

[G. Ann Campbell]

Hi Michael,

To address your question about the rule's default severity: 

Rule severities are set using a truth table:

For Code Smells, which this rule is, the impact is judged by whether or not violating the rule could lead a maintainer to introduce a bug. As Brian described, this could indeed lead a maintainer to introduce a bug. So this is a high-impact rule. But the likelihood of introducing a bug isn't deemed high, so we arrive at a Critical severity.

HTH,

Ann

[Michel Pawlak]

Hi Ann,

According to the table, if "the likelihood isn't deemed high", shouldn't be the severity set to "High"?

Michel

[G. Ann Campbell]

Hi Michel,

Thanks for following up on this. It hit me that I sent the wrong truth table about a second before I opened this email. Here's the right one:

Ann

---

G. Ann CAMPBELL | SonarSource

Product Manager

http://sonarsource.com

--
You received this message because you are subscribed to a topic in the Google Groups "SonarQube" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sonarqube/VeQ57JTKFYg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sonarqube+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/bb735a89-10a6-4cb9-b6ab-ecb1cb0f7640%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Michael Piefel]

Hello Brian,

thank you for your comment. However, it seems you have not addressed a single issue of those that I mentioned. I have even listed cases where you actually want to change only one String literal of a bunch of otherwise identical ones.

Moreover, just putting everything into constants doesn’t help you with other common cases: In a Spring MVC, we moved a sub-tree and subsequently had to change a lot of strings like "participant/settings" and "participant/dashboard" into "user/settings" and "user/dashboard". Having these strings in constants does not help finding all places.

Bye,

   Mike

[Michael Piefel]

Am Mittwoch, 11. Januar 2017 20:46:06 UTC+1 schrieb G. Ann Campbell:

For Code Smells, which this rule is, the impact is judged by whether or not violating the rule could lead a maintainer to introduce a bug. As Brian described, this could indeed lead a maintainer to introduce a bug. So this is a high-impact rule. But the likelihood of introducing a bug isn't deemed high, so we arrive at a Critical severity.

Hi Ann,

almost everything may lead a maintainer to introducing a bug. Wrong indentation, unconventional identifier naming, variables declared in the wrong place in a file… So how do you actually decide on this?

Also, I do not understand the table: If violating the rule will not cause a maintainer to introduce a bug, what is the relevance of the ‘likelihood’ aspect?

Greetings,

   Mike

[G. Ann Campbell]

Hi Michael,

There's judgement in all of this. We try to base those  judgements on the average coder on the average day. For major and minor code smells likelihood is based on the likelihood of the Worst Thing actually coming to pass.

Ann

---

G. Ann CAMPBELL | SonarSource

Product Manager

http://sonarsource.com

--

You received this message because you are subscribed to a topic in the Google Groups "SonarQube" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sonarqube/VeQ57JTKFYg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sonarqube+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/d37bdff4-8b0d-4baf-8c71-a611642a96c4%40googlegroups.com.