# Best Practices

Rules which enforce generally accepted best practices.
Table of Contents

## 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
        }
    }

Use this rule by 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:

Name | Default Value | Description
--- | --- | ---
additionalAssertMethodPattern |  | A regular expression for one or more custom test assertion method patterns.

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>

## ApexUnitTestClassShouldHaveRunAs

Since: PMD 6.51.0

Priority: Medium (3)

Apex unit tests should include at least one runAs method. This makes the tests more robust, and independent from the user running it.

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

Example(s):

    @isTest
    private class TestRunAs {
       public static testMethod void testRunAs() {
            // Setup test data
            // Create a unique UserName
            String uniqueUserName = 'standarduser' + DateTime.now().getTime() + '@testorg.com';
            // This code runs as the system user
            Profile p = [SELECT Id FROM Profile WHERE Name='Standard User'];
            User u = new User(Alias = 'standt', Email='standarduser@testorg.com',
            EmailEncodingKey='UTF-8', LastName='Testing', LanguageLocaleKey='en_US',
            LocaleSidKey='en_US', ProfileId = p.Id,
            TimeZoneSidKey='America/Los_Angeles',
             UserName=uniqueUserName);

            System.runAs(u) {
                  // The following code runs as user 'u'
                  System.debug('Current User: ' + UserInfo.getUserName());
                  System.debug('Current Profile: ' + UserInfo.getProfileId());
              }
        }
    }

Use this rule by referencing it:

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

## 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 advises 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() {
        }
    }

Use this rule by 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();
        }
    }

Use this rule by referencing it:

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

## AvoidFutureAnnotation

Since: PMD 7.19.0

Priority: Medium Low (4)

Usage of the `@Future` annotation should be limited. The `@Future` annotation is a legacy way to execute asynchronous Apex code, but it has several limitations:

- Methods must be static
- Only primitive data types, primitive arrays, or collections of primitive data types can be parameters
- No way to monitor job status or handle failures
- No support for chaining jobs

Consider implementing the `Queueable` interface instead, which provides:

- Better error handling and monitoring capabilities
- Support for more complex data types
- Ability to chain jobs

See more here: https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_queueing_jobs.htm

This rule is defined by the following XPath expression:

    //Method/ModifierNode/Annotation[lower-case(@Name) = 'future']

Example(s):

    // Not recommended: Using @Future annotation
    public class FutureExample {
        @Future
        public static void futureMethod(String accountId) {
            // Process account asynchronously
            Account acc = [SELECT Id, Name FROM Account WHERE Id = :accountId];
            // ... process account
        }
    }

    // Recommended: Using Queueable interface with Finalizer to detect and handle job failure
    public class QueueableExample implements Queueable, Finalizer {
        private String accountId;

        public QueueableExample(String accountId) {
            this.accountId = accountId;
        }

        public void execute(QueueableContext context) {
            System.attachFinalizer(this);
            Account acc = [SELECT Id, Name FROM Account WHERE Id = :accountId];
            // ... process account
        }

        public void execute(FinalizerContext ctx) {
            if (ctx.getResult() == ParentJobResult.SUCCESS) {
                // Handle success
            } else {
                // Handle failure
            }
        }
    }

    // Usage:
    // System.enqueueJob(new QueueableExample(accountId));

Use this rule by referencing it:

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

## 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 unchangeable(UndeletableType param) {
            // ...
        }
    }

Use this rule by 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) {
                // ...
            }
        }
    }

Use this rule by 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:

Name | Default Value | Description
--- | --- | ---
strictMode | false | If true, mark statements that use the DEBUG enum of LoggingLevel.

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>

## QueueableWithoutFinalizer

Since: PMD 7.8.0

Priority: Low (5)

Detects when the Queueable interface is used but a Finalizer is not attached. It is best practice to call the `System.attachFinalizer(Finalizer f)` method within the `execute` method of a class which implements the `Queueable` interface. Without attaching a Finalizer, there is no way of designing error recovery actions should the Queueable action fail.

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

Example(s):

    // Incorrect code, does not attach a finalizer.
    public class UserUpdater implements Queueable {
        public List<User> usersToUpdate;

        public UserUpdater(List<User> usersToUpdate) {
            this.usersToUpdate = usersToUpdate;
        }

        public void execute(QueueableContext context) { // no Finalizer is attached
            update usersToUpdate;
        }
    }

    // Proper code, attaches a finalizer.
    public class UserUpdater implements Queueable, Finalizer {
        public List<User> usersToUpdate;

        public UserUpdater(List<User> usersToUpdate) {
            this.usersToUpdate = usersToUpdate;
        }

        public void execute(QueueableContext context) {
            System.attachFinalizer(this);
            update usersToUpdate;
        }

        public void execute(FinalizerContext ctx) {
            if (ctx.getResult() == ParentJobResult.SUCCESS) {
                // Handle success
            } else {
                // Handle failure
            }
        }
    }

Use this rule by referencing it:

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

## 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);
        }

Use this rule by referencing it:

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