Home » Blog » Salesforce PMD: Apex Errors and Warnings

Salesforce PMD: Apex Errors and Warnings

In the first installment in this series, we 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 we’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 the priority order we use at LWS. Our additional notes are in bold. In parenthesis is PMD’s default severity – 1, highest to 5, lowest:

1 Change absolutely required. Behavior is critically broken/buggy

  • 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.
  • 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.
  • OperationWithLimitsInLoop (3): Database operations in loops will almost certainly cause problems either now or later.

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

  • ApexUnitTestClassShouldHaveAsserts (3): a unit test should include at least one assertion. We’re huge fans of great testing. A unit test without an assertion is at best simply verifying that code doesn’t throw an exception. Not good enough.
  • ApexCSRF (3): Having DML operations in constructors or initializers can have unexpected side effects. Fact. This one has bitten us several times.
  • AvoidDirectAccessTriggerMap (3): Avoid directly accessing Trigger.old and Trigger.new as it can lead to a bug. Trigger code should always be bulkified.
  • AvoidHardcodingId (3): These will break when the code is deployed to another org.
  • OverrideBothEqualsAndHashcode (3): Override both public boolean Object.equals(Object other), and public int Object.hashCode(), or override neither.
  • ApexOpenRedirect (3): Checks against redirects to user-controlled locations.
  • ApexSharingViolations (3): Detect classes declared without explicit sharing mode if DML methods are used.
  • ApexSOQLInjection (3): Detects the usage of untrusted / unescaped variables in DML queries.
  • ApexSuggestUsingNamedCred (3): Detects hardcoded credentials used in requests to an endpoint.

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

  • AvoidGlobalModifier (3): Global classes should be avoided. Use the most restrictive modifiers possible (i.e. public instead of global, private instead of public).
  • AvoidLogicInTrigger (3). Code in triggers is not reusable and is difficult to test.
  • Enforcing braces around code blocks will definitely help you avoid bugs
    • ForLoopsMustUseBraces (3)
    • IfElseStmtsMustUseBraces (3)
    • IfStmtsMustUseBraces (3)
    • WhileLoopsMustUseBraces (3)
  • These rules are complexity heuristics. At LWS, we don’t give these a high priority because we prefer larger, well-organized classes.
    • CognitiveComplexity (3): Methods that are highly complex are difficult to read and more costly to maintain.
    • CyclomaticComplexity (3): The complexity of methods directly affects maintenance costs and readability.
    • ExcessiveClassLength (3): Excessive class file lengths are usually indications that the class may be burdened with excessive…
    • NcssConstructorCount (3): This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of l…
    • NcssMethodCount (3): This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of l…
    • NcssTypeCount (3): This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of l…
    • StdCyclomaticComplexity (3): Complexity directly affects maintenance costs is determined by the number of decision points in a…
  • EmptyStatementBlock (3): Empty block statements serve no purpose and should be removed.
  • 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.
  • ApexBadCrypto (3): The rule makes sure you are using randomly generated IVs and keys for ‘Crypto’ calls.
  • 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.
  • ApexInsecureEndpoint (3): Checks against accessing endpoints under plain http.
  • ApexXSSFromEscapeFalse (3): Reports on calls to ‘addError’ with disabled escaping.
  • ApexXSSFromURLParam (3): All values obtained from URL parameters should be escaped / sanitized. In our experience, Ids are fine; numbers are fine; dates are fine…

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

  • UnusedLocalVariable (3): Detects when a local variable is declared and/or assigned but not used. Unused code makes your work more difficult to maintain
  • The default naming conventions enforce case, which aids in readability
    • ClassNamingConventions (1): Configurable naming conventions for type declarations.
    • FieldNamingConventions (1): Configurable naming conventions for field declarations.
    • FormalParameterNamingConventions (1): Configurable naming conventions for formal parameters of methods.
    • LocalVariableNamingConventions (1): Configurable naming conventions for local variable declarations.
    • MethodNamingConventions (1): Configurable naming conventions for method declarations.
    • PropertyNamingConventions (1): Configurable naming conventions for property declarations.
  • These seem to be somewhat arbitrary and correct at least as often as they are problematic
    • AvoidDeeplyNestedIfStmts (3): Avoid creating deeply nested if-then statements since they are harder to read and error-prone.
    • ExcessiveParameterList (3): Methods with numerous parameters are a challenge to maintain.
  • ExcessivePublicCount (3): Classes with large numbers of public methods and attributes require disproportionate testing effort.
  • TooManyFields (3): Classes that have too many fields can become unwieldy.
  • EmptyTryOrFinallyBlock (3): Avoid empty try or finally blocks. Unused code should be removed.

5 Change highly-optional. Nice to have

  • MethodWithSameNameAsEnclosingClass (3): Non-constructor methods should not have the same name as the enclosing class.
  • Sometimes empty statements make code more readable…
    • EmptyIfStmt (3): Empty If Statement finds instances where a condition is checked but nothing is done about it.
    • EmptyWhileStmt (3): Empty While Statement finds all instances where a while statement does nothing.
  • These rules are complexity heuristics. At LWS, we prefer larger, well-organized classes to the hundreds of classes typical in a large org.
    • CognitiveComplexity (3): Methods that are highly complex are difficult to read and more costly to maintain.
    • CyclomaticComplexity (3): The complexity of methods directly affects maintenance costs and readability.
    • ExcessiveClassLength (3): Excessive class file lengths are usually indications that the class may be burdened with excessive…
    • NcssConstructorCount (3): This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of l…
    • NcssMethodCount (3): This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of l…
    • NcssTypeCount (3): This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of l…
    • StdCyclomaticComplexity (3): Complexity directly affects maintenance costs is determined by the number of decision points in a…

5+ Not needed

  • 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.
  • ApexUnitTestMethodShouldHaveIsTestAnnotation (3): Apex test methods should have @isTest annotation. testMethod is deprecated
  • DebugsShouldUseLoggingLevel (3): The first parameter of System.debug should specify the level
  • FieldDeclarationsShouldBeAtStart (3): Field declarations should appear before method declarations within a class. We disagree here. Declaring properties and variables close to their use is best practice.
  • OneDeclarationPerLine (1): Apex allows several variables to be declared on one line.
  • 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. This seems like a non-issue.

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