Uploaded image for project: 'EJBCA'
  1. EJBCA
  2. ECA-8086

Revise active CheckStyle and PMD checks

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • 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)

      https://magnum-ci.primekey.com/job/EJBCA/job/EE_DEB9_OpenJDK8_SCAN/lastSuccessfulBuild/checkstyle/#typeContent

      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)

      https://magnum-ci.primekey.com/job/EJBCA/job/EE_DEB9_OpenJDK8_SCAN/lastSuccessfulBuild/pmd/#typeContent

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              samuel Samuel Lidén Borell
              Reporter:
              samuel Samuel Lidén Borell
              Verified by:
              Andrey Sergeev
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Time Tracking

                  Estimated:
                  Original Estimate - 1 day Original Estimate - 1 day
                  1d
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 1 day, 4 hours
                  1d 4h