Best Practices

ApexAssertionsShouldIncludeMessage

Since: PMD 6.13.0

Priority: Medium (3)

The second parameter of System.assert/third parameter of System.assertEquals/System.assertNotEquals is a message. Having a second/third parameter provides more information and makes it easier to debug the test failure and improves the readability of test output.

This rule is defined by the following Java class: net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexAssertionsShouldIncludeMessageRule

Example(s):

@isTest
public class Foo {
    @isTest
    static void methodATest() {
        System.assertNotEquals('123', o.StageName); // not good
        System.assertEquals('123', o.StageName, 'Opportunity stageName is wrong.'); // good
        System.assert(o.isClosed); // not good
        System.assert(o.isClosed, 'Opportunity is not closed.'); // good
    }
}

This rule has the following properties:

Use this rule with the default properties by just referencing it:

<rule ref="category/apex/bestpractices.xml/ApexAssertionsShouldIncludeMessage" />

ApexUnitTestClassShouldHaveAsserts

Since: PMD 5.5.1

Priority: Medium (3)

Apex unit tests should include at least one assertion. This makes the tests more robust, and using assert with messages provide the developer a clearer idea of what the test does. Custom assert method invocation patterns can be specified using the ‘additionalAssertMethodPattern’ property if required.

This rule is defined by the following Java class: net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexUnitTestClassShouldHaveAssertsRule

Example(s):

@isTest
public class Foo {
    public static testMethod void testSomething() {
        Account a = null;
        // This is better than having a NullPointerException
        // System.assertNotEquals(a, null, 'account not found');
        a.toString();
    }
}

This rule has the following properties:

Use this rule with the default properties by just referencing it:

<rule ref="category/apex/bestpractices.xml/ApexUnitTestClassShouldHaveAsserts" />

Use this rule and customize it:

<rule ref="category/apex/bestpractices.xml/ApexUnitTestClassShouldHaveAsserts">
    <properties>
        <property name="additionalAssertMethodPattern" value="" />
    </properties>
</rule>

ApexUnitTestMethodShouldHaveIsTestAnnotation

Since: PMD 6.13.0

Priority: Medium (3)

Apex test methods should have @isTest annotation instead of the testMethod keyword, as testMethod is deprecated. Salesforce advices to use @isTest annotation for test classes and methods.

This rule is defined by the following XPath expression:

//Method[ModifierNode[@DeprecatedTestMethod = true()]]

Example(s):

@isTest
private class ATest {
    @isTest
    static void methodATest() {
    }
    static void methodBTest() {
    }
    @isTest static void methodCTest() {
        System.assert(1==2);
    }
    static testmethod void methodCTest() {
        System.debug('I am a debug statement');
    }
    private void fetchData() {
    }
}

This rule has the following properties:

Use this rule with the default properties by just referencing it:

<rule ref="category/apex/bestpractices.xml/ApexUnitTestMethodShouldHaveIsTestAnnotation" />

ApexUnitTestShouldNotUseSeeAllDataTrue

Since: PMD 5.5.1

Priority: Medium (3)

Apex unit tests should not use @isTest(seeAllData=true) because it opens up the existing database data for unexpected modification by tests.

This rule is defined by the following Java class: net.sourceforge.pmd.lang.apex.rule.bestpractices.ApexUnitTestShouldNotUseSeeAllDataTrueRule

Example(s):

@isTest(seeAllData = true)
public class Foo {
    public static testMethod void testSomething() {
        Account a = null;
        // This is better than having a NullPointerException
        // System.assertNotEquals(a, null, 'account not found');
        a.toString();
    }
}

This rule has the following properties:

Use this rule with the default properties by just referencing it:

<rule ref="category/apex/bestpractices.xml/ApexUnitTestShouldNotUseSeeAllDataTrue" />

AvoidGlobalModifier

Since: PMD 5.5.0

Priority: Medium (3)

Global classes should be avoided (especially in managed packages) as they can never be deleted or changed in signature. Always check twice if something needs to be global. Many interfaces (e.g. Batch) required global modifiers in the past but don’t require this anymore. Don’t lock yourself in.

This rule is defined by the following Java class: net.sourceforge.pmd.lang.apex.rule.bestpractices.AvoidGlobalModifierRule

Example(s):

global class Unchangeable {
    global UndeletableType unchangable(UndeletableType param) {
        // ...
    }
}

This rule has the following properties:

Use this rule with the default properties by just referencing it:

<rule ref="category/apex/bestpractices.xml/AvoidGlobalModifier" />

AvoidLogicInTrigger

Since: PMD 5.5.0

Priority: Medium (3)

As triggers do not allow methods like regular classes they are less flexible and suited to apply good encapsulation style. Therefore delegate the triggers work to a regular class (often called Trigger handler class).

See more here: https://developer.salesforce.com/page/Trigger_Frameworks_and_Apex_Trigger_Best_Practices

This rule is defined by the following Java class: net.sourceforge.pmd.lang.apex.rule.bestpractices.AvoidLogicInTriggerRule

Example(s):

trigger Accounts on Account (before insert, before update, before delete, after insert, after update, after delete, after undelete) {
    for(Account acc : Trigger.new) {
        if(Trigger.isInsert) {
            // ...
        }

        // ...

        if(Trigger.isDelete) {
            // ...
        }
    }
}

This rule has the following properties:

Use this rule with the default properties by just referencing it:

<rule ref="category/apex/bestpractices.xml/AvoidLogicInTrigger" />

DebugsShouldUseLoggingLevel

Since: PMD 6.18.0

Priority: Medium (3)

The first parameter of System.debug, when using the signature with two parameters, is a LoggingLevel enum.

Having the Logging Level specified provides a cleaner log, and improves readability of it.

This rule is defined by the following XPath expression:

//MethodCallExpression[lower-case(@FullMethodName)='system.debug'][count(*)=2
    or ($strictMode=true() and count(*)=3 and lower-case(VariableExpression/@Image)='debug')]

Example(s):

@isTest
public class Foo {
    @isTest
    static void bar() {
        System.debug('Hey this code executed.'); // not good
        System.debug(LoggingLevel.WARN, 'Hey, something might be wrong.'); // good
        System.debug(LoggingLevel.DEBUG, 'Hey, something happened.'); // not good when on strict mode
    }
}

This rule has the following properties:

Use this rule with the default properties by just referencing it:

<rule ref="category/apex/bestpractices.xml/DebugsShouldUseLoggingLevel" />

Use this rule and customize it:

<rule ref="category/apex/bestpractices.xml/DebugsShouldUseLoggingLevel">
    <properties>
        <property name="strictMode" value="false" />
    </properties>
</rule>

UnusedLocalVariable

Since: PMD 6.23.0

Priority: Low (5)

Detects when a local variable is declared and/or assigned but not used.

This rule is defined by the following Java class: net.sourceforge.pmd.lang.apex.rule.bestpractices.UnusedLocalVariableRule

Example(s):

public Boolean bar(String z) {
        String x = 'some string'; // not used

        String y = 'some other string'; // used in the next line
        return z.equals(y);
    }

This rule has the following properties:

Use this rule with the default properties by just referencing it:

<rule ref="category/apex/bestpractices.xml/UnusedLocalVariable" />