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