Details
-
Type:
Improvement
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: None
-
Component/s: None
-
Labels:
-
Provenance:Refactorization
-
Issue discovered during:Jenkins
-
Sprint:EJBCA Team Bob - 2019 w21, EJBCA Team Bob - 2019 w24
Description
I looked through the errors from CheckStyle and PMD. Many of the checks seem to give only or mostly false positives (some are always wrong for us). We should discuss which ones to disable.
For CheckStyle I think we can disable (should be discussed)
AvoidInlineConditionalsCheck (not bad code IMHO)
AvoidNestedBlocksCheck (not bad code IMHO)
ConstantNameCheck (matches non-compile time constant fields also, such as "log", so too much spam)
EmptyBlockCheck (matches empty blocks with a comment why it is empty...)
InnerAssignmentCheck (useful code pattern sometimes)
InterfaceIsTypeCheck (this will give false positives with interfaces that describe a behavior)
LeftCurlyCheck (lots of false positives for getters and setters)
MethodNameCheck (conflicts with EcaQa test case naming)
MissingSwitchDefaultCheck (this is actually a good coding practice, because then the compiler can check for missing enum cases)
NoWhitespaceAfterCheck (contrary to our usual style for array initializers)
OperatorWrapCheck (controversial topic, some find this less error prone other find it more error prone. Also contrary to most of the EJBCA code)
RegexpSinglelineCheck (false positives on indented blank lines, which in turn is controversial)
TodoCommentCheck (false positives with TODO comments that have an issue number)
TypeNameCheck (false positives with WS exception names)
For PMD I think we can disable (should be discussed)
AvoidBranchingStatementAsLastInLoop (false positive if the loop contains "continue" statements)
AvoidUsingShortType (maybe a useless optimization sometimes, but not worth a warning)
AvoidUsingVolatile (needed in some multi-threaded code)
ClassNamingConventions (should be fixed to match *Tools and *Constants as utility classes also, or be disabled)
DoNotCallGarbageCollectionExplicitly (only done 3 times, and motivated in these cases)
EmptyMethodInAbstractClassShouldBeAbstract (many legitimate uses of this)
FieldNamingConventions (false positives with non-compile time constants, such as "log" variables)
MethodNamingConventions (conflicts with EcaQa test case naming)
ReturnEmptyArrayRatherThanNull (legitimate uses of this)
SystemPrintln (false positives in CLI and in main() methods)