# Best Practices

Rules which enforce generally accepted 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:

Name | Default Value | Description | Multivalued
--- | --- | --- | ---
cc_categories | Bug Risk | Deprecated Code Climate Categories | yes. Delimiter is ‘|’.
cc_remediation_points_multiplier | 100 | Deprecated Code Climate Remediation Points multiplier | no
cc_block_highlighting | false | Deprecated Code Climate Block Highlighting | no

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:

Name | Default Value | Description | Multivalued
--- | --- | --- | ---
cc_categories | Bug Risk | Deprecated Code Climate Categories | yes. Delimiter is ‘|’.
cc_remediation_points_multiplier | 100 | Deprecated Code Climate Remediation Points multiplier | no
cc_block_highlighting | false | Deprecated Code Climate Block Highlighting | no
additionalAssertMethodPattern |  | A regular expression for one or more custom test assertion method patterns. | no

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:

Name | Default Value | Description | Multivalued
--- | --- | --- | ---
cc_categories | Style | Deprecated Code Climate Categories | yes. Delimiter is ‘|’.
cc_remediation_points_multiplier | 1 | Deprecated Code Climate Remediation Points multiplier | no
cc_block_highlighting | false | Deprecated Code Climate Block Highlighting | no

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:

Name | Default Value | Description | Multivalued
--- | --- | --- | ---
cc_categories | Bug Risk | Deprecated Code Climate Categories | yes. Delimiter is ‘|’.
cc_remediation_points_multiplier | 100 | Deprecated Code Climate Remediation Points multiplier | no
cc_block_highlighting | false | Deprecated Code Climate Block Highlighting | no

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:

Name | Default Value | Description | Multivalued
--- | --- | --- | ---
cc_categories | Style | Deprecated Code Climate Categories | yes. Delimiter is ‘|’.
cc_remediation_points_multiplier | 100 | Deprecated Code Climate Remediation Points multiplier | no
cc_block_highlighting | false | Deprecated Code Climate Block Highlighting | no

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:

Name | Default Value | Description | Multivalued
--- | --- | --- | ---
cc_categories | Style | Deprecated Code Climate Categories | yes. Delimiter is ‘|’.
cc_remediation_points_multiplier | 200 | Deprecated Code Climate Remediation Points multiplier | no
cc_block_highlighting | false | Deprecated Code Climate Block Highlighting | no

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:

Name | Default Value | Description | Multivalued
--- | --- | --- | ---
cc_categories | Style | Deprecated Code Climate Categories | yes. Delimiter is ‘|’.
cc_remediation_points_multiplier | 1 | Deprecated Code Climate Remediation Points multiplier | no
cc_block_highlighting | false | Deprecated Code Climate Block Highlighting | no
strictMode | false | If true, mark statements that use the DEBUG enum of LoggingLevel. | no

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:

Name | Default Value | Description | Multivalued
--- | --- | --- | ---
cc_categories | Style | Deprecated Code Climate Categories | yes. Delimiter is ‘|’.
cc_remediation_points_multiplier | 1 | Deprecated Code Climate Remediation Points multiplier | no
cc_block_highlighting | false | Deprecated Code Climate Block Highlighting | no

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

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