Define Sonar Rules
Last modified by Vincent Massol on 2024/11/19 16:14
Vincent MassolDescription
The goal of this page is to define the list of quality rules to execute on Sonar for the XWiki builds.
The strategy I'm following is to enable all available rules (as they existed on 2014-08-17 in Sonar 4.4) and then start removing some, one by one, when we don't want to apply them or when they yield invalid results for us.
I'm writing down here the reasons why some rules were excluded.
First we're excluding all "Deprecated" rules.
We're also excluding all "java8" checks FTM since we're using java7.
In addition, we're excluding the following rules:
| Sonar Rule Name | Sonar Rule ID | Reason for excluding it |
|---|---|---|
| Header | checkstyle:com.puppycrawl.tools.checkstyle.checks.header.HeaderCheck | Already checked by our Maven License plugin |
| Regexp Header | checkstyle:com.puppycrawl.tools.checkstyle.checks.header.RegexpHeaderCheck | Already checked by our Maven License plugin |
| Copyright and license headers should be defined in all source files | squid:S1451 | Already checked by our Maven License plugin. In addition, I couldn't make it work since copy/pasting our license headers resulted in only the first 5 lines being copied |
| Strict Duplicate Code | checkstyle:com.puppycrawl.tools.checkstyle.checks.duplicates.StrictDuplicateCodeCheck | Already covered by "Duplicated blocks" (common-java:DuplicatedBlocks). In addition doesn't exclude license headers by default... |
| Left curly braces should be located at the end of lines of code | squid:LeftCurlyBraceEndLineCheck | Our rule is to have curly braces at end of lines except for classes and methods and apparently there's no way to configure this check. |
| Left curly braces should be located at the beginning of lines of code | squid:LeftCurlyBraceStartLineCheck | Our rule is to have curly braces at end of lines except for classes and methods and apparently there's no way to configure this check. |
| Right curly brace and next "else", "catch" and "finally" keywords should be located on two different lines | squid:RightCurlyBraceDifferentLineAsNextBlockCheck | Our rule is to have them on the same line |
| Dataflow Anomaly Analysis | pmd:DataflowAnomalyAnalysis | Categorized as controversial by PMD. Seems complex to use it properly. |
| Bean Members Should Serialize | pmd:BeanMembersShouldSerialize | Too many false positives for us, we don't expect our component impls to be serializable for example. |
| Long Variable | pmd:LongVariable | We want to favor clarity and thus we don't put a cap on variable names |
| Missing Constructor | checkstyle:com.puppycrawl.tools.checkstyle.checks.coding.MissingCtorCheck | We don't want to force having a constructor |
| Public types, methods and fields (API) should be documented with Javadoc | squid:UndocumentedApi | Normally we should use this rule instead of the Checkstyle ones ("Javadoc Type", "Javadoc Variable" and "Javadoc Method") which are deprecated. However it doesn't let us say that we want to apply it only on classes not in the internal package. |
| Illegal Catch | checkstyle:com.puppycrawl.tools.checkstyle.checks.coding.IllegalCatchCheck | We want to be able to catch Exception |
| Avoid Catching Generic Exception | pmd:AvoidCatchingGenericException | We want to be able to catch Exception |
| JUnit Test Contains Too Many Asserts | pmd-unit-tests:JUnitTestContainsTooManyAsserts | We've not been following this practice. Not sure it's that good. |
| Comment Size | pmd:CommentSize | Generates false positives with license headers |
| Comment Required | pmd:CommentRequired | Generates false positives for documenting private class fields which we don't want to enforce |
| Design For Extension | checkstyle:com.puppycrawl.tools.checkstyle.checks.design.DesignForExtensionCheck | We're not doing this |
| Final Local Variable | checkstyle:com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck | Better use PMD's "Local variable could be final" (pmd:LocalVariableCouldBeFinal) if we want to do this in the future (see below in the next table) |
| Javadoc Package | checkstyle:com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck | A bit too much probably... |
| Magic Number | checkstyle:com.puppycrawl.tools.checkstyle.checks.coding.MagicNumberCheck | Better use Squid's "Magic numbers should not be used" (squid:S109) if we want to do this in the future (see below in the next table) |
| Loggers should be "private static final" and should share a naming convention | squid:S1312 | Cannot use since we use field logger injection |
| JUnit tests should include an assert | pmd-unit-tests:JUnitTestsShouldIncludeAssert | Good rule but doesn't work well; for ex if a method is called that does the assert, it's not taken into account |
| Guard Log Statement | pmd:GuardLogStatement | Not needed with SLF4J |
| Illegal Token | checkstyle:com.puppycrawl.tools.checkstyle.checks.coding.IllegalTokenCheck | Useful in "for" loops for example |
| Exception handlers should preserve the original exception | squid:S1166 | Sometimes we want to catch an exception and not rethrow it |
| Guard Debug Logging | pmd:GuardDebugLogging | Not needed with SLF4J and leads to false positives in lots of cases |
| Test class without test cases (JUnit 3.x only) | pmd-unit-tests:TestClassWithoutTestCases | We're using JUnit 4.x |
| Guard Log Statement Java Util | pmd:GuardLogStatementJavaUtil | Not fully needed with SLF4J and leads to false positives |
| Use ConcurrentHashMap | pmd:UseConcurrentHashMap | Potentially interesting but leads to too many false positives. For ex when inside a method we construct a Map to be returned; there's no issue about concurrency in this case. |
| An abstract class should have both abstract and concrete methods | squid:S1694 | We've not been following this practice and I don't see why it's better. |
| Consecutive Appends Should Reuse | pmd:ConsecutiveAppendsShouldReuse | Prevents ability to comment each line and it would be strange that the JIT doesn't recompile it at runtime. |
Rules that are currently excluded but could be potentially VOTEd for inclusion in the future:
| Sonar Rule Name | Sonar Rule ID | Notes |
|---|---|---|
| Law Of Demeter | pmd:LawOfDemeter | We've not been following this design practice so far. |
| Insufficient comment density | common-java:InsufficientCommentDensity | Interesting rule. |
| Short Variable | pmd:ShortVariable | Interesting rule since var names like i or nb are not the best for clarity. |
| Confusing Ternary | pmd:ConfusingTernary | Would make our code more readable and consistent (right now 215 violations in commons). |
| Immutable Field | pmd:ImmutableField | Could be interesting |
| Avoid Literals In If Condition | pmd:AvoidLiteralsInIfCondition | |
| Local variable could be final | pmd:LocalVariableCouldBeFinal | Could be interesting |
| Inner Type Last | checkstyle:com.puppycrawl.tools.checkstyle.checks.design.InnerTypeLastCheck | |
| Magic numbers should not be used | squid:S109 | |
| Generic exceptions Error, RuntimeException, Throwable and Exception should never be thrown | squid:S00112 | Nice but costly to apply |
| Null Assignment | pmd:NullAssignment | Could lead to false positives for setting to null for garbage collecting |
| Require This | checkstyle:com.puppycrawl.tools.checkstyle.checks.coding.RequireThisCheck | We've been mostly following this rule but not enforcing it. We need to decide what we wish to do about it |
