False positives for: "String literals should not be used" (squid:S1192)

[alix....@gmail.com]

Official rule description:

String literals should not be duplicated

 squid:S1192

Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.

On the other hand, constants can be referenced from many places, but only need to be updated in a single place.

Suggestion

I argue that logging statements should be exempted from this rule. An example is the repetition of:

logger.debug("This is unexpected. Bla bla bla");

Following the rule you would have to extract an awkward constant when this statement is repeated:

private static final String MESSAGE_THIS_IS_UNEXPECTED_BLA_BLA = "This is unexpected. Bla bla bla";

...

logger.debug(MESSAGE_THIS_IS_UNEXPECTED_BLA_BLA);

This post is a duplicate of my post on StackOverflow, where I unfortunately didn't receive any feedback: https://stackoverflow.com/questions/44561234/usefulness-of-rule-string-literals-should-not-be-duplicated-squids1192

[Nicolas Peru]

Hi, 

I don't think this is a question suited for stackoverlow as this is quite arguable and requires discussion. 

So your problem is issue raised on duplicated message logged. Your solution to this issue is to extract a constant (as suggested by the rule). While I agree with you that this does not seems a nice solution I don't think the solution is to exclude this duplication from the rule. The duplication is actually correct but the nicest refactoring in this case would be to wrap the logging into a method call IMHO : 

private void logDebugMessage() {

   logger.debug("This is unexpected. Bla bla bla");

}

to reduce  the duplication. 

Cheers, 

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/5ea942f4-83b9-47da-aac8-6c40952f0ef7%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Nicolas Peru | SonarSource

Senior Developer

http://twitter.com/benzonico

http://sonarsource.com

[alix....@gmail.com]

Hi, 

I was not aware of this Google group when I posted the original message but I will make sure to bring up this sort of discussions here in the future.

While I agree with you that the violation could be silenced that way, I don't think that the intent of the rule is duplication. 

This is a rule with critical severity, meaning things could go terribly wrong if you don't do anything about it. A log message that is duplicated does not seem to me to be that severe.

So for the sake of keeping down the number of high severity false negatives I still argue to exempt log messages from the rule.

[s53...@gmail.com]

Hi, I think there are definately cases that should be exlcudeable - so far short literals are excluded. But it would be much more efficient if you could exclude specific words or phrases.

If you write a database application, you will have "JOIN ", "LEFT JOIN ", "FROM ", ... again and again and so some real issues would be buried in 99% false positives of this kind. I don't think that declaring some constants would be useful here. Every column, every table, every ... would have to be a constant. There's no benefit in such cases, but the code is filled up with constants.

Cheers,
Stefan.

[Daniel Schwarz]

Hi Stefan,

I tend to agree with your use case.

However, for the sake of completeness, I want to point out some alternatives, that might not only avoid those sonar issues, but could also (depending on your environment) improve your solution:

* Write your own SQL-Helper classes, that assemble SQL (do not repeat yourself)

* Use a library for assembling SQL (I have no experience here, but googling led me to jOOQ which seems to be free for some database management systems)

* Extract SQL templates from your code, by either using a template framework (like Apache Velocity) or using a lightweight persistence framework (like MyBatis)

* Use an Object-relational mapping framework (based on JPA or Hibernate for example)

By the way: SonarQube internally uses MyBatis

Cheers!

Daniel

--

You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/cddc03b3-0640-4c92-9e78-842a6cc64d18%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Daniel Schwarz | SonarSource

Developer

https://sonarsource.com

[s53...@gmail.com]

Hi Daniel,

we already have gathered the SQLs in some subproject, but there are near 1000 SQLs, maybe more (it's a huge project with over 1 million LOCs) - so to keep track of what's going on and find, what you search, we won't change the architecture here.

So, I'd still suggest to add a whitelist of Strings that may be duplicated without notice.

Thanks, cheers,
Stefan.

[alix....@gmail.com]

Hi,

I want to bump this issue as it is very unfortunate that a rule with critical severity complains about logging statements which obviously is not something to be worried about.

People I talk to are annoyed and choose to suppress the rule completely which I think is really sad as the rule catches, potentially, very nasty bugs.

Please prioritize this!

Regards

Alix

[deathkn...@gmail.com]

Bump,

Can anyone help with I want to replicate this rule thou altering it when there is a line reference to something like "log" or "logger" can anyone help with this??
Struggling to find any one else who has done something similar and would appreciate the help

[Nicolas Peru]

Hi, 

you may want to start reading this : https://docs.sonarqube.org/display/PLUG/Writing+Custom+Java+Rules+101

Cheers, 

--

You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/1499442a-9f4e-4b9c-98bc-0db9eb1007e8%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Nicolas Peru | SonarSource

Senior Developer

https://twitter.com/benzonico

https://sonarsource.com