Avoid the "Clean Code Shock" with PMD
Your new year resolution includes "Write clean Apex code". So you run PMD with a full ruleset and get shocked by the number of violations. You drop the resolution in a blink.
Don't boil the Ocean
Even a journey of a thousand miles starts with a single step, so let's break down the task into manageable chunks to divide and rule.
There are 2 dimensions you can use: Type of code and priority levels. Using them you can turn your Clean Code journey into manageable stages.
Code Types
- Legacy code: all code that doesn't fall in any of the two other categories
- Changed code: code that needs change due to business requirements
- New code: new code written for new or changed functionality (applies to copy & paste too)
Priority Levels
- 1 = security and performance, will fail build
- 2 = bad code, will fail build
- 3 & 4 = hard to maintain code, will generate warning
- 5 = ugly code, will generate hint
PMD rules for code types should have different priorities. A different number of tests will fail a build:
- 11 for legacy code (all around performance and security)
- 33 for changed code
- 44 for new code
This will require to run PMD with different rulesets on subsets of your code base
A pragmatic rule set
|Rule Set |Rule Name |Legacy code|Updated Code|New Code|
|--------------|--------------------------------------|:---------:|:----------:|:------:|
|Best Practices|ApexUnitTestShouldNotUseSeeAllDataTrue| 3 | 2 | 2 |
| |ApexUnitTestClassShouldHaveAsserts | 3 | 2 | 2 |
| |AvoidLogicInTrigger | 2 | 2 | 2 |
| |AvoidGlobalModifier | 5 | 4 | 3 |
|Code Style |ClassNamingConventions | 5 | 3 | 1 |
| |ForLoopsMustUseBraces | 4 | 2 | 1 |
| |IfStmtsMustUseBraces | 4 | 2 | 1 |
| |IfElseStmtsMustUseBraces | 5 | 2 | 1 |
| |MethodNamingConventions | 5 | 3 | 1 |
| |OneDeclarationPerLine | 5 | 2 | 1 |
| |VariableNamingConventions | 5 | 3 | 1 |
| |WhileLoopsMustUseBraces | 4 | 2 | 1 |
|Design |AvoidDeeplyNestedIfStmts | 3 | 2 | 2 |
| |CyclomaticComplexity | 3 | 3 | 2 |
| |ExcessiveClassLength | 3 | 3 | 2 |
| |ExcessiveParameterList | 3 | 3 | 2 |
| |ExcessivePublicCount | 3 | 3 | 2 |
| |NcssConstructorCount | 3 | 2 | 2 |
| |NcssMethodCount | 3 | 3 | 2 |
| |NcssTypeCount | 3 | 3 | 2 |
| |StdCyclomaticComplexity | 3 | 3 | 2 |
| |TooManyFields | 3 | 3 | 3 |
|Error Prone |AvoidDirectAccessTriggerMap | 2 | 2 | 1 |
| |AvoidHardcodingId | 2 | 2 | 1 |
| |EmptyCatchBlock | 2 | 2 | 2 |
| |EmptyIfStmt | 3 | 2 | 2 |
| |EmptyStatementBlock | 3 | 3 | 2 |
| |EmptyTryOrFinallyBlock | 3 | 3 | 2 |
| |EmptyWhileStmt | 3 | 3 | 2 |
| |MethodWithSameNameAsEnclosingClass | 3 | 3 | 2 |
|Performance |AvoidDmlStatementsInLoops | 1 | 1 | 1 |
| |AvoidSoslInLoops | 1 | 1 | 1 |
| |AvoidSoqlInLoops | 1 | 1 | 1 |
|Security |ApexCRUDViolation | 3 | 2 | 1 |
| |ApexInsecureEndpoint | 2 | 2 | 2 |
| |ApexOpenRedirect | 3 | 2 | 2 |
| |ApexSharingViolations | 3 | 3 | 2 |
| |ApexSOQLInjection | 1 | 1 | 1 |
| |ApexXSSFromURLParam | 1 | 1 | 1 |
| |VfCsrf | 1 | 1 | 1 |
| |ApexXSSFromEscapeFalse | 3 | 2 | 1 |
| |ApexBadCrypto | 1 | 1 | 1 |
| |ApexCSRF | 3 | 2 | 2 |
| |ApexDangerousMethods | 3 | 2 | 2 |
| |ApexSuggestUsingNamedCred | 1 | 1 | 1 |
| |VfUnescapeEl | 1 | 1 | 1 |
As usual: YMMV
You need to tune the rule to match your code quality of your existing base. The most controversial would be the "updated code" rule.If a business requirement mandates to add 3-4 lines, but the PMD rule would fail on complexity, it becomes hard to justify to upfront work, so you might, initially, have more relaxed rules in place.
Posted by Stephan H Wissel on 02 January 2019 | Comments (0) | categories: Apex PMD Salesforce