PMD: Improving Apex Code Quality - Errors & Warnings

How we got here

In the first installment in this series, I talked briefly about the utility and importance of static code analysis. In summary, static analysis tools such as PMD improve quality and save money by finding defects quickly. In order to get started using PMD, that first article covered installation and execution of the tool. This week I’ll cover some of the more common PMD errors and warnings in Apex code, and why you should heed them.

Current Rules - prioritized and annotated

PMD is highly configurable. You and your team can decide which infractions are “errors”, which are “warnings”, and which will be ignored. You can also modify existing PMD rules or create your own. Here are the non-deprecated PMD rules, sorted in my own personal priority order. Besides the new priorities, my additions are in bold. In parenthesis is PMD’s default severity - 5, highest to 1, lowest:

5 Change absolutely required. Behavior is critically broken/buggy

• Database operations in loops will almost certainly cause problems either now or later (AvoidDmlStatementsInLoops (3), AvoidSoqlInLoops (3), AvoidSoslInLoops (3))
• EmptyCatchBlock (3): Empty Catch Block finds instances where an exception is caught, but nothing is done. HUGE red flag. It’s almost never a good idea to swallow an exception
• AvoidHardcodingId (3): When deploying… These will break when deployed to another org.
• ApexUnitTestClassShouldHaveAsserts (3): a unit test should include at least one assertion. I’m a huge fan of great testing. A unit test without an assertion is at best simply verifying that code doesn’t throw an exception. Not good enough.

4 Change highly recommended. Behavior is quite likely to be broken/buggy

• AvoidLogicInTrigger (3). Code in triggers is not reusable and is difficult to test.
• ApexCSRF (3): Having DML operations in constructors or initializers can have unexpected side effects. Fact. This one has bitten me several times.
• ApexBadCrypto (3): The rule makes sure you are using randomly generated IVs and keys for ‘Crypto’ calls.
• ApexUnitTestShouldNotUseSeeAllDataTrue (3): a unit test should not use @isTest(seeAllData=true). This is very rarely needed in Apex development and may cause problems when tests are deployed.

3 Change recommended. Behavior is confusing, perhaps buggy, and/or against standards/best practices

• Enforcing braces around code blocks will definitely help you avoid bugs (ForLoopsMustUseBraces (3), IfElseStmtsMustUseBraces (3), IfStmtsMustUseBraces (3), WhileLoopsMustUseBraces (3))
• UnusedLocalVariable (5): Detects when a local variable is declared and/or assigned but not used. Unused code makes your work more difficult to maintain
• AvoidDirectAccessTriggerMap (3): Avoid directly accessing Trigger.old and Trigger.new as it can lead to a bug. (when outside of a loop, of course)
• ApexCRUDViolation (3): The rule validates you are checking for access permissions before a SOQL/SOSL/DML operation.
• ApexDangerousMethods (3): Checks against calling dangerous methods. The primary “danger” here is sending sensitive data to System.debug.
• ApexOpenRedirect (3): Checks against redirects to user-controlled locations.
• ApexSharingViolations (3): Detect classes declared without explicit sharing mode if DML methods are used.

2 Change optional. Behavior is not likely to be buggy, but more just flies in the face of standards/style/good taste

• ApexInsecureEndpoint (3): Checks against accessing endpoints under plain http.
• ApexSOQLInjection (3): Detects the usage of untrusted / unescaped variables in DML queries.
• ApexSuggestUsingNamedCred (3): Detects hardcoded credentials used in requests to an endpoint.
• ApexXSSFromEscapeFalse (3): Reports on calls to ‘addError’ with disabled escaping.
• MethodWithSameNameAsEnclosingClass (3): Non-constructor methods should not have the same name as the enclosing class. I wasn’t aware this was possible. Definitely confusing and a bad idea.
• AvoidGlobalModifier (3): Global classes should be avoided. Use the most restrictive modifiers possible (i.e. public instead of global, private instead of public).
• FieldDeclarationsShouldBeAtStart (3): Field declarations should appear before method declarations within a class. Consistent code organization helps with readability
• ApexXSSFromURLParam (3): All values obtained from URL parameters should be escaped / sanitized. False. Ids are fine. Numbers are fine. Dates are fine...

1 Change highly-optional. Nice to have

• ApexAssertionsShouldIncludeMessage (3): the last parameter of System.assert can be used to document the assertion. Caution - be sure to make this a truly useful comment.
• TestMethodsMustBeInTestClasses (3): Test methods marked as a testMethod or annotated with @IsTest, but not residing in a test class. Enforced by the compiler according to API number.
• ApexUnitTestMethodShouldHaveIsTestAnnotation (3): Apex test methods should have @isTest annotation. testMethod is deprecated
• These rules are complexity heuristics. Personally, I prefer larger, well-organized classes to the hundreds of classes typical in a large org. (CognitiveComplexity (3), CyclomaticComplexity (3), ExcessiveClassLength (3), ExcessivePublicCount (3), NcssConstructorCount (3), NcssMethodCount (3), NcssTypeCount (3), StdCyclomaticComplexity (3), TooManyFields (3)
• The default naming conventions enforce case, which aids in readability (ClassNamingConventions (1), FieldNamingConventions (1), FormalParameterNamingConventions (1), LocalVariableNamingConventions (1), MethodNamingConventions (1), PropertyNamingConventions (1), OneDeclarationPerLine (1)

0 Not needed

• Often, empty statements make code more readable… (EmptyIfStmt (3), EmptyStatementBlock (3), EmptyTryOrFinallyBlock (3), EmptyWhileStmt (3))
• ApexDoc (3): This rule validates that: ApexDoc comments are present. Fewer comments are an indicator of great code. Commenting rules too often result in lots of unnecessary work and useless comments.
• AvoidNonExistentAnnotations (3): Apex supported non-existent annotations for legacy reasons. Seems like a non-issue.
• DebugsShouldUseLoggingLevel (3): The first parameter of System.debug should specify the level
• These seem to be somewhat arbitrary and correct at least as often as they are problematic (AvoidDeeplyNestedIfStmts (3), ExcessiveParameterList (3)

There you have it: a prioritized list of all the current PMD Apex rules. This list changes slightly with nearly every release, of course, so put together your own list and revisit it frequently. Visit PMDs rule reference as needed.

Hope to see you next time to explore rule prioritization, customization, and authoring. Until then, happy coding!

***

Steve Cox is the Development Center of Excellence Lead at Xede Consulting Group, and a senior software developer with over 20 years of experience. Steve's thought leadership includes a book: Salesforce Defect Domination: A handbook for finding, fixing, and preventing defects in Force.com development, Dreamforce presentations, and blog posts.