{0} should be in PascalCase: @{1}
+Apex, while case-insensitive, benefits from a consistent code style to improve readability and maintainability. + Enforcing PascalCase for annotations aligns with the established conventions and reduces ambiguity - promoting a unified coding standard.
+ // Incorrect:
+ @istest
+ private static void fooShouldBar() {
+ //...
+ }
+
+ // Correct:
+ @IsTest
+ private static void fooShouldBar() {
+ //...
+ }
+
+ //Incorrect:
+ @testvisible
+ private boolean doSomething = false;
+
+
+ //Correct:
+ @TestVisible
+ private boolean doSomething = false;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.
+ @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
+ }
+ }The rule makes sure you are using randomly generated IVs and keys for Crypto calls.
+Hard-wiring these values greatly compromises the security of encrypted data.
public without sharing class Foo {
+ Blob hardCodedIV = Blob.valueOf('Hardcoded IV 123');
+ Blob hardCodedKey = Blob.valueOf('0000000000000000');
+ Blob data = Blob.valueOf('Data to be encrypted');
+ Blob encrypted = Crypto.encrypt('AES128', hardCodedKey, hardCodedIV, data);
+ }The rule validates you are checking for access permissions before a SOQL/SOSL/DML operation. +Since Apex runs by default in system mode not having proper permissions checks results in escalation of +privilege and may produce runtime errors. This check forces you to handle such scenarios.
+Since Winter '23 (API Version 56) you can enforce user mode for database operations by using
+WITH USER_MODE in SOQL. This makes Apex to respect Field-level security (FLS) and object
+permissions of the running user. When using user mode, no violation is reported by this rule.
By default, the rule allows access checks can be performed using system Apex provisions such as
+DescribeSObjectResult.isAccessible/Createable/etc., the SOQL WITH SECURITY_ENFORCED clause,
+or using the open source Force.com ESAPI
+class library. Because it is common to use authorization facades to assist with this task, the
+rule also allows configuration of regular expression-based patterns for the methods used to
+authorize each type of CRUD operation. These pattern are configured via the following properties:
createAuthMethodPattern/createAuthMethodTypeParamIndex - a pattern for the method used for create authorization and an optional 0-based index of the parameter passed to that method that denotes the SObjectType being authorized for create.readAuthMethodPattern/readAuthMethodTypeParamIndex - a pattern for the method used for read authorization and an optional 0-based index of the parameter passed to that method that denotes the SObjectType being authorized for read.updateAuthMethodPattern/updateAuthMethodTypeParamIndex - a pattern for the method used for update authorization and an optional 0-based index of the parameter passed to that method that denotes the SObjectType being authorized for update.deleteAuthMethodPattern/deleteAuthMethodTypeParamIndex - a pattern for the method used for delete authorization and an optional 0-based index of the parameter passed to that method that denotes the SObjectType being authorized for delete.undeleteAuthMethodPattern/undeleteAuthMethodTypeParamIndex - a pattern for the method used for undelete authorization and an optional 0-based index of the parameter passed to that method that denotes the SObjectType being authorized for undelete.mergeAuthMethodPattern/mergeAuthMethodTypeParamIndex - a pattern for the method used for merge authorization and an optional 0-based index of the parameter passed to that method that denotes the SObjectType being authorized for merge.The following example shows how the rule can be configured for the
+sirono-common
+AuthorizationUtil class:
<rule ref="category/apex/security.xml/ApexCRUDViolation" message="Validate CRUD permission before SOQL/DML operation">
+ <priority>3</priority>
+ <properties>
+ <property name="createAuthMethodPattern" value="AuthorizationUtil\.(is|assert)(Createable|Upsertable)"/>
+ <property name="readAuthMethodPattern" value="AuthorizationUtil\.(is|assert)Accessible"/>
+ <property name="updateAuthMethodPattern" value="AuthorizationUtil\.(is|assert)(Updateable|Upsertable)"/>
+ <property name="deleteAuthMethodPattern" value="AuthorizationUtil\.(is|assert)Deletable"/>
+ <property name="undeleteAuthMethodPattern" value="AuthorizationUtil\.(is|assert)Undeletable"/>
+ <property name="mergeAuthMethodPattern" value="AuthorizationUtil\.(is|assert)Mergeable"/>
+ </properties>
+ </rule>Note: This rule will produce false positives for VF getter methods. In VF getters the access permission +check happens automatically and is not needed explicitly. However, the rule can't reliably determine +whether a getter is a VF getter or not and reports a violation in any case. In such cases, the violation +should be suppressed.
+ public class Foo {
+ public Contact foo(String status, String ID) {
+
+ // validate you can actually query what you intend to retrieve
+ Contact c = [SELECT Status__c FROM Contact WHERE Id=:ID WITH SECURITY_ENFORCED];
+
+ // Make sure we can update the database before even trying
+ if (!Schema.sObjectType.Contact.fields.Status__c.isUpdateable()) {
+ return null;
+ }
+
+ c.Status__c = status;
+ update c;
+ return c;
+ }
+ }Having DML operations in Apex class constructor or initializers can have unexpected side effects: + By just accessing a page, the DML statements would be executed and the database would be modified. + Just querying the database is permitted.
+In addition to constructors and initializers, any method called init is checked as well.
Salesforce Apex already protects against this scenario and raises a runtime exception.
+Note: This rule has been moved from category "Security" to "Error Prone" with PMD 6.21.0, since + using DML in constructors is not a security problem, but crashes the application.
+ public class Foo {
+ // initializer
+ {
+ insert data;
+ }
+
+ // static initializer
+ static {
+ insert data;
+ }
+
+ // constructor
+ public Foo() {
+ insert data;
+ }
+ }Checks against calling dangerous methods.
+For the time being, it reports:
+FinancialForce's Configuration.disableTriggerCRUDSecurity(). Disabling CRUD security opens the door to several attacks and requires manual validation, which is unreliable.System.debug passing sensitive data as parameter, which could lead to exposure of private data. public class Foo {
+ public Foo() {
+ Configuration.disableTriggerCRUDSecurity();
+ }
+ }This rule validates that:
+reportPrivate and reportProtected.reportMissingDescription.reportProperty is enabled. By setting reportProperty to false, you can ignore missing comments on properties.Method overrides and tests are both exempted from having ApexDoc.
+ /**
+ * @description Hello World
+ */
+ public class HelloWorld {
+ /**
+ * @description Bar
+ * @return Bar
+ */
+ public Object bar() { return null; }
+ }Checks against accessing endpoints under plain http. You should always use +https for security.
+ public without sharing class Foo {
+ void foo() {
+ HttpRequest req = new HttpRequest();
+ req.setEndpoint('http://localhost:com');
+ }
+ }Checks against redirects to user-controlled locations. This prevents attackers from +redirecting users to phishing sites.
+ public without sharing class Foo {
+ String unsafeLocation = ApexPage.getCurrentPage().getParameters.get('url_param');
+ PageReference page() {
+ return new PageReference(unsafeLocation);
+ }
+ }Detects the usage of untrusted / unescaped variables in DML queries.
+ public class Foo {
+ public void test1(String t1) {
+ Database.query('SELECT Id FROM Account' + t1);
+ }
+ }Detect classes declared without explicit sharing mode if DML methods are used. This +forces the developer to take access restrictions into account before modifying objects.
+ public without sharing class Foo {
+ // DML operation here
+ }Detects hardcoded credentials used in requests to an endpoint.
+You should refrain from hardcoding credentials:
+Instead, you should use Named Credentials and a callout endpoint.
+For more information, you can check this
+ public class Foo {
+ public void foo(String username, String password) {
+ Blob headerValue = Blob.valueOf(username + ':' + password);
+ String authorizationHeader = 'BASIC ' + EncodingUtil.base64Encode(headerValue);
+ req.setHeader('Authorization', authorizationHeader);
+ }
+ }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.
+ @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();
+ }
+ }Apex unit tests should include at least one runAs method. This makes the tests more robust, and independent from the +user running it.
+ @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());
+ }
+ }
+ }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.
@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() {
+ }
+ }Apex unit tests should not use @isTest(seeAllData=true) because it opens up the existing database data for unexpected modification by tests.
+ @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();
+ }
+ }Reports on calls to addError with disabled escaping. The message passed to addError
+will be displayed directly to the user in the UI, making it prime ground for XSS
+attacks if unescaped.
public without sharing class Foo {
+ Trigger.new[0].addError(vulnerableHTMLGoesHere, false);
+ }Makes sure that all values obtained from URL parameters are properly escaped / sanitized +to avoid XSS attacks.
+ public without sharing class Foo {
+ String unescapedstring = ApexPage.getCurrentPage().getParameters.get('url_param');
+ String usedLater = unescapedstring;
+ }Boolean parameters in a system's API can make method calls difficult to understand and + maintain. They often indicate that a method is doing more than one thing and + could benefit from being split into separate methods with more descriptive + names.
+This rule flags any boolean parameters found in public or global methods, + encouraging developers to use more expressive alternatives such as enums, + separate methods, or configuration objects.
+ // Violates the rule: Uses a Boolean parameter
+ public class MyClass {
+ public static void doSomething(Boolean isSomething) {
+ if (isSomething == true) {
+ // Do something
+ } else {
+ // Do something else, or maybe do nothing if isSomething is null?
+ }
+ }
+ }
+
+ // Compliant code: Two separate methods
+ public class MyClass {
+ public static void doSomething() {
+ // Do something
+ }
+
+ public static void doSomethingElse() {
+ // Do something else
+ }
+ } public void setFlag(Boolean strict) { ... } // violation
+
+ // compliant
+ public void enableStrictChecking() { ... }
+ public void disableStrictChecking() { ... }Debug statements contribute to longer transactions and consume Apex CPU time even when debug logs are not being captured.
+When possible make use of other debugging techniques such as the Apex Replay Debugger and Checkpoints that could cover most use cases.
+For other valid use cases that the statement is in fact valid make use of the @SuppressWarnings annotation or the //NOPMD comment.
public class Foo {
+ public void bar() {
+ Account acc = [SELECT Name, Owner.Name FROM Account LIMIT 1];
+ System.debug(accs); // will get reported
+ }
+
+ @SuppressWarnings('PMD.AvoidDebugStatements')
+ public void baz() {
+ try {
+ Account myAccount = bar();
+ } catch (Exception e) {
+ System.debug(LoggingLevel.ERROR, e.getMessage()); // good to go
+ }
+ }
+ }Avoid creating deeply nested if-then statements since they are harder to read and error-prone to maintain.
+ public class Foo {
+ public void bar(Integer x, Integer y, Integer z) {
+ if (x>y) {
+ if (y>z) {
+ if (z==x) {
+ // !! too deep
+ }
+ }
+ }
+ }
+ }Avoid directly accessing Trigger.old and Trigger.new as it can lead to a bug. Triggers should be bulkified and iterate through the map to handle the actions for each item separately.
+ trigger AccountTrigger on Account (before insert, before update) {
+ Account a = Trigger.new[0]; //Bad: Accessing the trigger array directly is not recommended.
+
+ for ( Account a : Trigger.new ) {
+ //Good: Iterate through the trigger.new array instead.
+ }
+ }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.
+ global class Unchangeable {
+ global UndeletableType unchangable(UndeletableType param) {
+ // ...
+ }
+ }When deploying Apex code between sandbox and production environments, or installing Force.com AppExchange packages, + it is essential to avoid hardcoding IDs in the Apex code. By doing so, if the record IDs change between environments, + the logic can dynamically identify the proper data to operate against and not fail.
+ public without sharing class Foo {
+ void foo() {
+ //Error - hardcoded the record type id
+ if (a.RecordTypeId == '012500000009WAr') {
+ //do some logic here.....
+ } else if (a.RecordTypeId == '0123000000095Km') {
+ //do some logic here for a different record type...
+ }
+ }
+ }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
+ 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) {
+ // ...
+ }
+ }
+ }Apex supported non existent annotations for legacy reasons. + In the future, use of such non-existent annotations could result in broken apex code that will not compile. + This will prevent users of garbage annotations from being able to use legitimate annotations added to Apex in the future. + A full list of supported annotations can be found at https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_classes_annotation.htm
+ @NonExistentAnnotation public class ClassWithNonexistentAnnotation {
+ @NonExistentAnnotation public void methodWithNonExistentAnnotation() {
+ // ...
+ }
+ }{0} queries without a where or limit statement
+When working with very large amounts of data, unfiltered SOQL or SOSL queries can quickly cause + governor limit + exceptions.
+ public class Something {
+ public static void main( String[] as ) {
+ Account[] accs1 = [ select id from account ]; // Bad
+ Account[] accs2 = [ select id from account limit 10 ]; // better
+
+ List<List<SObject>> searchList = [FIND 'map*' IN ALL FIELDS RETURNING Account (Id, Name), Contact, Opportunity, Lead]; // bad
+ }
+ }Database.[x]Result instance variables can cause serialization errors between successive batch iterations.
+Using instance variables of the following types (or collections of these types) within a stateful batch class can cause serialization errors between batch iterations:
+Database.DeleteResultDatabase.EmptyRecycleBinResultDatabase.MergeResultDatabase.SaveResultDatabase.UndeleteResultDatabase.UpsertResultThis error occurs inconsistently and asynchronously with an obscure error message - making it particularly challenging to troubleshoot. + See this issue for more details.
+These errors can be avoided by marking the variable as static, transient, or using a different + data type that is safe to serialize.
+ // Violating
+ public class Example implements Database.Batchable<SObject>, Database.Stateful {
+ List<Database.SaveResult> results = new List<Database.SaveResult>(); // This can cause failures
+
+ public Database.Querylocator start(Database.BatchableContext context) {
+ return Database.getQueryLocator('SELECT Id FROM Account');
+ }
+
+ public void execute(Database.BatchableContext context, List<SObject> scope) {
+ Database.SaveResult[] saveResults = Database.update(scope, false);
+ results.addAll(saveResults);
+ }
+
+ public void finish(database.BatchableContext context) {
+ }
+ }
+
+ // Compliant
+ public class Example implements Database.Batchable<SObject>, Database.Stateful {
+ List<StatefulResult> results = new List<StatefulResult>(); // Use a different custom type to persist state
+
+ public Database.Querylocator start(Database.BatchableContext context) {
+ return Database.getQueryLocator('SELECT Id FROM Account');
+ }
+
+ public void execute(Database.BatchableContext context, List<SObject> scope) {
+ Database.SaveResult[] saveResults = Database.update(scope, false);
+ for (Database.SaveResult result : saveResults) {
+ results.add(new StatefulResult(result));
+ }
+ }
+
+ public void finish(database.BatchableContext context) {
+ }
+
+ }
+
+ public class StatefulResult {
+ private Boolean isSuccess;
+ private Id id;
+ private Database.Error[] errors;
+
+ public StatefulResult(Database.SaveResult result) {
+ isSuccess = result.isSuccess();
+ id = result.getId();
+ errors = result.getErrors();
+ }
+
+ public Boolean isSuccess() {
+ return isSuccess;
+ }
+
+ public Id getId() {
+ return id;
+ }
+
+ public Database.Error[] getErrors() {
+ return errors;
+ }
+ }{0} name '{1}' doesn't match '{2}'
+Configurable naming conventions for type declarations. This rule reports + type declarations which do not match the regex that applies to their + specific kind (e.g. enum or interface). Each regex can be configured through + properties.
+By default this rule uses the standard Apex naming convention (Pascal case).
+ public class FooClass { } // This is in pascal case, so it's ok
+
+ public class fooClass { } // This will be reported unless you change the regex{0} '{1}' has a{2} cognitive complexity of {3}, current threshold is {4}
+Methods that are highly complex are difficult to read and more costly to maintain. If you include too much decisional +logic within a single method, you make its behavior hard to understand and more difficult to modify.
+Cognitive complexity is a measure of how difficult it is for humans to read and understand a method. Code that contains +a break in the control flow is more complex, whereas the use of language shorthands doesn't increase the level of +complexity. Nested control flows can make a method more difficult to understand, with each additional nesting of the +control flow leading to an increase in cognitive complexity.
+Information about Cognitive complexity can be found in the original paper here: +https://www.sonarsource.com/docs/CognitiveComplexity.pdf
+By default, this rule reports methods with a complexity of 15 or more. Reported methods should be broken down into less +complex components.
+ public class Foo {
+ // Has a cognitive complexity of 0
+ public void createAccount() {
+ Account account = new Account(Name = 'PMD');
+ insert account;
+ }
+
+ // Has a cognitive complexity of 1
+ public Boolean setPhoneNumberIfNotExisting(Account a, String phone) {
+ if (a.Phone == null) { // +1
+ a.Phone = phone;
+ update a;
+ return true;
+ }
+
+ return false;
+ }
+
+ // Has a cognitive complexity of 4
+ public void updateContacts(List<Contact> contacts) {
+ List<Contact> contactsToUpdate = new List<Contact>();
+
+ for (Contact contact : contacts) { // +1
+ if (contact.Department == 'Finance') { // +2 (nesting = 1)
+ contact.Title = 'Finance Specialist';
+ contactsToUpdate.add(contact);
+ } else if (contact.Department == 'Sales') { // +1
+ contact.Title = 'Sales Specialist';
+ contactsToUpdate.add(contact);
+ }
+ }
+
+ update contactsToUpdate;
+ }
+ }{0} '{1}' has a{2} cyclomatic complexity of {3}.
+The complexity of methods directly affects maintenance costs and readability. Concentrating too much decisional logic +in a single method makes its behaviour hard to read and change.
+Cyclomatic complexity assesses the complexity of a method by counting the number of decision points in a method, +plus one for the method entry. Decision points are places where the control flow jumps to another place in the +program. As such, they include all control flow statements, such as 'if', 'while', 'for', and 'case'.
+Generally, numbers ranging from 1-4 denote low complexity, 5-7 denote moderate complexity, 8-10 denote +high complexity, and 11+ is very high complexity. By default, this rule reports methods with a complexity >= 10. +Additionally, classes with many methods of moderate complexity get reported as well once the total of their +methods' complexities reaches 40, even if none of the methods was directly reported.
+Reported methods should be broken down into several smaller methods. Reported classes should probably be broken down +into subcomponents.
+ public class Complicated {
+ public void example() { // This method has a cyclomatic complexity of 12
+ int x = 0, y = 1, z = 2, t = 2;
+ boolean a = false, b = true, c = false, d = true;
+ if (a && b || b && d) {
+ if (y == z) {
+ x = 2;
+ } else if (y == t && !d) {
+ x = 2;
+ } else {
+ x = 2;
+ }
+ } else if (c && d) {
+ while (z < y) {
+ x = 2;
+ }
+ } else {
+ for (int n = 0; n < t; n++) {
+ x = 2;
+ }
+ }
+ }
+ }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.
+ @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 finds DescribeSObjectResults which could have been loaded eagerly via SObjectType.getDescribe().
When using SObjectType.getDescribe() or Schema.describeSObjects() without supplying a SObjectDescribeOptions,
+implicitly it will be using SObjectDescribeOptions.DEFAULT and then all
+child relationships will be loaded eagerly regardless whether this information is needed or not.
+This has a potential negative performance impact. Instead SObjectType.getDescribe(options)
+or Schema.describeSObjects(SObjectTypes, options)
+should be used and a SObjectDescribeOptions should be supplied. By using
+SObjectDescribeOptions.DEFERRED the describe attributes will be lazily initialized at first use.
Lazy loading DescribeSObjectResult on picklist fields is not always recommended. The lazy loaded
+describe objects might not be 100% accurate. It might be safer to explicitly use
+SObjectDescribeOptions.FULL in such a case. The same applies when you need the same DescribeSObjectResult
+to be consistent across different contexts and API versions.
Properties:
+noDefault: The behavior of SObjectDescribeOptions.DEFAULT changes from API Version 43 to 44: With API Version 43, the attributes are loaded eagerly. With API Version 44, they are loaded lazily. Simply using SObjectDescribeOptions.DEFAULT doesn't automatically make use of lazy loading. (unless "Use Improved Schema Caching" critical update is applied, SObjectDescribeOptions.DEFAULT does fallback to lazy loading) With this property enabled, such usages are found. You might ignore this, if you can make sure, that you don't run a mix of API Versions. public class Foo {
+ public static void bar(List<Account> accounts) {
+ if (Account.SObjectType.getDescribe(SObjectDescribeOptions.DEFERRED).isCreateable()) {
+ insert accounts;
+ }
+ }
+ }Empty Catch Block finds instances where an exception is caught, but nothing is done. + In most circumstances, this swallows an exception which should either be acted on + or reported.
+ public void doSomething() {
+ ...
+ try {
+ insert accounts;
+ } catch (DmlException dmle) {
+ // not good
+ }
+ }Empty If Statement finds instances where a condition is checked but nothing is done about it.
+ public class Foo {
+ public void bar(Integer x) {
+ if (x == 0) {
+ // empty!
+ }
+ }
+ }Empty block statements serve no purpose and should be removed.
+ public class Foo {
+
+ private Integer _bar;
+
+ public void setBar(Integer bar) {
+ // empty
+ }
+
+ }Avoid empty try or finally blocks - what's the point?
+ public class Foo {
+ public void bar() {
+ try {
+ // empty !
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
+ }
+ }
+
+ public class Foo {
+ public void bar() {
+ try {
+ Integer x=2;
+ } finally {
+ // empty!
+ }
+ }
+ }Empty While Statement finds all instances where a while statement does nothing. + If it is a timing loop, then you should use Thread.sleep() for it; if it is + a while loop that does a lot in the exit expression, rewrite it to make it clearer.
+ public void bar(Integer a, Integer b) {
+ while (a == b) {
+ // empty!
+ }
+ }Excessive class file lengths are usually indications that the class may be burdened with excessive +responsibilities that could be provided by external classes or functions. In breaking these methods +apart the code becomes more managable and ripe for reuse.
+ public class Foo {
+ public void bar1() {
+ // 1000 lines of code
+ }
+ public void bar2() {
+ // 1000 lines of code
+ }
+ public void bar3() {
+ // 1000 lines of code
+ }
+ public void barN() {
+ // 1000 lines of code
+ }
+ }Methods with numerous parameters are a challenge to maintain, especially if most of them share the +same datatype. These situations usually denote the need for new objects to wrap the numerous parameters.
+ // too many arguments liable to be mixed up
+ public void addPerson(Integer birthYear, Integer birthMonth, Integer birthDate, Integer height, Integer weight, Integer ssn) {
+ // ...
+ }
+ // preferred approach
+ public void addPerson(Date birthdate, BodyMeasurements measurements, int ssn) {
+ // ...
+ }{0} has {1} public methods, attributes, and properties (limit: {2})
+Classes with large numbers of public methods, attributes, and properties require disproportionate testing efforts +since combinatorial side effects grow rapidly and increase risk. Refactoring these classes into +smaller ones not only increases testability and reliability but also allows new variations to be +developed easily.
+ public class Foo {
+ public String value;
+ public Bar something;
+ public Variable var;
+ // [... more more public attributes ...]
+
+ public void doWork() {}
+ public void doMoreWork() {}
+ public void doWorkAgain() {}
+ // [... more more public methods ...]
+
+ public String property1 { get; set; }
+ // [... more more public properties ...]
+ }{0}' should be before method declarations in its class
+Field declarations should appear before method declarations within a class.
+ class Foo {
+ public Integer someField; // good
+
+ public void someMethod() {
+ }
+
+ public Integer anotherField; // bad
+ }{0} name '{1}' doesn't match '{2}'
+Configurable naming conventions for field declarations. This rule reports variable declarations + which do not match the regex that applies to their specific kind ---e.g. constants (static final), + static field, final field. Each regex can be configured through properties.
+By default this rule uses the standard Apex naming convention (Camel case).
+ public class Foo {
+ Integer instanceField; // This is in camel case, so it's ok
+
+ Integer INSTANCE_FIELD; // This will be reported unless you change the regex
+ }Avoid using 'for' statements without using surrounding braces. If the code formatting or +indentation is lost then it becomes difficult to separate the code being controlled +from the rest.
+ for (int i = 0; i < 42; i++) // not recommended
+ foo();
+
+ for (int i = 0; i < 42; i++) { // preferred approach
+ foo();
+ }{0} name '{1}' doesn't match '{2}'
+Configurable naming conventions for formal parameters of methods. + This rule reports formal parameters which do not match the regex that applies to their + specific kind (e.g. method parameter, or final method parameter). Each regex can be + configured through properties.
+By default this rule uses the standard Apex naming convention (Camel case).
+ public class Foo {
+ public bar(Integer methodParameter) { } // This is in camel case, so it's ok
+
+ public baz(Integer METHOD_PARAMETER) { } // This will be reported unless you change the regex
+ }Avoid using if..else statements without using surrounding braces. If the code formatting +or indentation is lost then it becomes difficult to separate the code being controlled +from the rest.
+ // this is OK
+ if (foo) x++;
+
+ // but this is not
+ if (foo)
+ x = x+1;
+ else
+ x = x-1;Avoid using if statements without using braces to surround the code block. If the code +formatting or indentation is lost then it becomes difficult to separate the code being +controlled from the rest.
+ if (foo) // not recommended
+ x++;
+
+ if (foo) { // preferred approach
+ x++;
+ }In the Summer '21 release, a mandatory security update enforces access modifiers on Apex properties in + Lightning component markup. The update prevents access to private or protected Apex getters from Aura + and Lightning Web Components.
+ public class Foo {
+ @AuraEnabled
+ public Integer counter { private get; set; } // Violating - Private getter is inaccessible to Lightning components
+
+ @AuraEnabled
+ public static Foo bar()
+ {
+ Foo foo = new Foo();
+ foo.counter = 2;
+ return foo;
+ }
+ } public class Foo {
+ @AuraEnabled
+ public Integer counter { protected get; set; } // Violating - Protected getter is inaccessible to Lightning components
+
+ @AuraEnabled
+ public static Foo bar()
+ {
+ Foo foo = new Foo();
+ foo.counter = 2;
+ return foo;
+ }
+ } public class Foo {
+ @AuraEnabled
+ public Integer counter { get; set; } // Compliant - Public getter is accessible to Lightning components
+
+ @AuraEnabled
+ public static Foo bar()
+ {
+ Foo foo = new Foo();
+ foo.counter = 2;
+ return foo;
+ }
+ }{0} name '{1}' doesn't match '{2}'
+Configurable naming conventions for local variable declarations. + This rule reports variable declarations which do not match the regex that applies to their + specific kind (e.g. local variable, or final local variable). Each regex can be configured through + properties.
+By default this rule uses the standard Apex naming convention (Camel case).
+ public class Foo {
+ public Foo() {
+ Integer localVariable; // This is in camel case, so it's ok
+
+ Integer LOCAL_VARIABLE; // This will be reported unless you change the regex
+ }
+ }{0} name '{1}' doesn't match '{2}'
+Configurable naming conventions for method declarations. This rule reports + method declarations which do not match the regex that applies to their + specific kind (e.g. static method, or test method). Each regex can be + configured through properties.
+By default this rule uses the standard Apex naming convention (Camel case).
+ public class Foo {
+ public void instanceMethod() { } // This is in camel case, so it's ok
+
+ public void INSTANCE_METHOD() { } // This will be reported unless you change the regexNon-constructor methods should not have the same name as the enclosing class.
+ public class MyClass {
+ // this is OK because it is a constructor
+ public MyClass() {}
+ // this is bad because it is a method
+ public void MyClass() {}
+ }{0}
+This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of lines +of code for a given constructor. NCSS ignores comments, and counts actual statements. Using this algorithm, +lines of code that are split are counted as one.
+ public class Foo extends Bar {
+ //this constructor only has 1 NCSS lines
+ public Foo() {
+ super();
+
+
+
+
+ super.foo();
+ }
+ }{0}()' has an NCSS line count of {1} (limit: {2})
+This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of lines +of code for a given method. NCSS ignores comments, and counts actual statements. Using this algorithm, +lines of code that are split are counted as one.
+ public class Foo extends Bar {
+ //this method only has 1 NCSS lines
+ public Integer method() {
+ super.method();
+
+
+
+ return 1;
+ }
+ }{0}
+This rule uses the NCSS (Non-Commenting Source Statements) algorithm to determine the number of lines +of code for a given type. NCSS ignores comments, and counts actual statements. Using this algorithm, +lines of code that are split are counted as one.
+ //this class only has 6 NCSS lines
+ public class Foo extends Bar {
+ public Foo() {
+ super();
+
+
+
+
+
+ super.foo();
+ }
+ }Apex allows the use of several variables declaration of the same type on one line. However, it +can lead to quite messy code. This rule looks for several declarations on the same line.
+ Integer a, b; // not recommended
+
+ Integer a,
+ b; // ok by default, can be flagged setting the strictMode property
+
+ Integer a; // preferred approach
+ Integer b;This rule finds method calls inside loops that are known to be likely a performance issue. These methods should be +called only once before the loop.
+Schema class methods like Schema.getGlobalDescribe() +and Schema.describeSObjects() +might be slow depending on the size of your organization. Calling these methods repeatedly inside a loop creates +a potential performance issue.
+ public class GlobalDescribeExample {
+ // incorrect example
+ public void getGlobalDescribeInLoop() {
+ Set<String> fieldNameSet = new Set<String> {'Id'};
+ for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
+ // Schema.getGlobalDescribe() should be called only once before the for-loop
+ if (Schema.getGlobalDescribe().get(objectName).getDescribe().fields.getMap().containsKey(fieldNameOrDefaultValue.trim())) {
+ fieldNameSet.add(fieldNameOrDefaultValue);
+ }
+ }
+ }
+
+ // corrected example
+ public void getGlobalDescribeInLoopCorrected() {
+ Map<String, Schema.SObjectField> fieldMap = Schema.getGlobalDescribe().get(objectName).getDescribe().fields.getMap();
+ Set<String> fieldNameSet = new Set<String> {'Id'};
+ for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
+ if (fieldMap.containsKey(fieldNameOrDefaultValue.trim())) {
+ fieldNameSet.add(fieldNameOrDefaultValue);
+ }
+ }
+ }
+ } public class DescribeSObjectsExample {
+ // incorrect example
+ public void describeSObjectsInLoop() {
+ Set<String> fieldNameSet = new Set<String> {'Id'};
+ for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
+ Schema.DescribeSObjectResult dsr = Account.sObjectType.getDescribe();
+ if (Schema.describeSObjects(new List<String> { sObjectType })[0].fields.getMap().containsKey(fieldNameOrDefaultValue.trim())) {
+ fieldNameSet.add(fieldNameOrDefaultValue);
+ }
+ }
+ }
+
+ // corrected example
+ public void describeSObjectsInLoop() {
+ Map<String, Schema.SObjectField> fieldMap = Schema.describeSObjects(new List<String> { 'Account' })[0].fields.getMap();
+ Set<String> fieldNameSet = new Set<String> {'Id'};
+ for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
+ if (fieldMap.containsKey(fieldNameOrDefaultValue.trim())) {
+ fieldNameSet.add(fieldNameOrDefaultValue);
+ }
+ }
+ }
+ }Database class methods, DML operations, SOQL queries, SOSL queries, Approval class methods, Email sending, async scheduling or queueing within loops can cause governor limit exceptions. Instead, try to batch up the data into a list and invoke the operation once on that list of data outside the loop.
+ public class Something {
+ public void databaseMethodInsideOfLoop(List<Account> accounts) {
+ for (Account a : accounts) {
+ Database.insert(a);
+ }
+ }
+
+ public void dmlInsideOfLoop() {
+ for (Integer i = 0; i < 151; i++) {
+ Account account;
+ // ...
+ insert account;
+ }
+ }
+
+ public void soqlInsideOfLoop() {
+ for (Integer i = 0; i < 10; i++) {
+ List<Account> accounts = [SELECT Id FROM Account];
+ }
+ }
+
+ public void soslInsideOfLoop() {
+ for (Integer i = 0; i < 10; i++) {
+ List<List<SObject>> searchList = [FIND 'map*' IN ALL FIELDS RETURNING Account (Id, Name), Contact, Opportunity, Lead];
+ }
+ }
+
+ public void messageInsideOfLoop() {
+ for (Integer i = 0; i < 10; i++) {
+ Messaging.SingleEmailMessage email = new Messaging.SingleEmailMessage();
+ Messaging.sendEmail(new Messaging.SingleEmailMessage[]{email});
+ }
+ }
+
+ public void approvalInsideOfLoop(Account[] accs) {
+ for (Integer i = 0; i < 10; i++) {
+ Account acc = accs[i];
+ Approval.ProcessSubmitRequest req = new Approval.ProcessSubmitRequest();
+ req.setObjectId(acc.Id);
+ Approval.process(req);
+ Approval.lock(acc);
+ Approval.unlock(acc);
+ }
+ }
+
+ public void asyncInsideOfLoop() {
+ for (Integer i = 0; i < 10; i++) {
+ System.enqueueJob(new MyQueueable());
+ System.schedule('x', '0 0 0 1 1 ?', new MySchedule());
+ System.scheduleBatch(new MyBatch(), 'x', 1);
+ }
+ }
+ }Override both public Boolean equals(Object obj), and public Integer hashCode(), or override neither.
+ Even if you are inheriting a hashCode() from a parent class, consider implementing hashCode and explicitly
+ delegating to your superclass.
This is especially important when Using Custom Types in Map Keys and Sets.
+ public class Bar { // poor, missing a hashCode() method
+ public Boolean equals(Object o) {
+ // do some comparison
+ }
+ }
+ public class Baz { // poor, missing an equals() method
+ public Integer hashCode() {
+ // return some hash value
+ }
+ }
+ public class Foo { // perfect, both methods provided
+ public Boolean equals(Object other) {
+ // do some comparison
+ }
+ public Integer hashCode() {
+ // return some hash value
+ }
+ }{0} name '{1}' doesn't match '{2}'
+Configurable naming conventions for property declarations. This rule reports + property declarations which do not match the regex that applies to their + specific kind (e.g. static property, or instance property). Each regex can be + configured through properties.
+By default this rule uses the standard Apex naming convention (Camel case).
+ public class Foo {
+ public Integer instanceProperty { get; set; } // This is in camel case, so it's ok
+
+ public Integer INSTANCE_PROPERTY { get; set; } // This will be reported unless you change the regex
+ }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.
// 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
+ }
+ }
+ }{0} '{1}' has a Standard Cyclomatic Complexity of {2}.
+Complexity directly affects maintenance costs is determined by the number of decision points in a method +plus one for the method entry. The decision points include 'if', 'while', 'for', and 'case labels' calls. +Generally, numbers ranging from 1-4 denote low complexity, 5-7 denote moderate complexity, 8-10 denote +high complexity, and 11+ is very high complexity.
+ // This has a Cyclomatic Complexity = 12
+ public class Foo {
+ 1 public void example() {
+ 2 if (a == b || (c == d && e == f)) {
+ 3 if (a1 == b1) {
+ fiddle();
+ 4 } else if a2 == b2) {
+ fiddle();
+ } else {
+ fiddle();
+ }
+ 5 } else if (c == d) {
+ 6 while (c == d) {
+ fiddle();
+ }
+ 7 } else if (e == f) {
+ 8 for (int n = 0; n < h; n++) {
+ fiddle();
+ }
+ } else {
+ switch (z) {
+ 9 case 1:
+ fiddle();
+ break;
+ 10 case 2:
+ fiddle();
+ break;
+ 11 case 3:
+ fiddle();
+ break;
+ 12 default:
+ fiddle();
+ break;
+ }
+ }
+ }Test methods marked as a testMethod or annotated with @IsTest, + but not residing in a test class should be moved to a proper + class or have the @IsTest annotation added to the class.
+Support for tests inside functional classes was removed in Spring-13 (API Version 27.0), + making classes that violate this rule fail compile-time. This rule is mostly usable when + dealing with legacy code.
+ // Violating
+ private class TestClass {
+ @IsTest static void myTest() {
+ // Code here
+ }
+ }
+
+ private class TestClass {
+ static testMethod void myTest() {
+ // Code here
+ }
+ }
+
+ // Compliant
+ @IsTest
+ private class TestClass {
+ @IsTest static void myTest() {
+ // Code here
+ }
+ }
+
+ @IsTest
+ private class TestClass {
+ static testMethod void myTest() {
+ // Code here
+ }
+ }Classes that have too many fields can become unwieldy and could be redesigned to have fewer fields, +possibly through grouping related fields in new objects. For example, a class with individual +city/state/zip fields could park them within a single Address field.
+ public class Person {
+ // too many separate fields
+ Integer birthYear;
+ Integer birthMonth;
+ Integer birthDate;
+ Double height;
+ Double weight;
+ }
+
+ public class Person {
+ // this is more manageable
+ Date birthDate;
+ BodyMeasurements measurements;
+ }{0} namespace. Please choose a different name.
+This rule finds Apex classes, enums, and interfaces that have the same name as a class, enum, or interface in the System
+ or Schema namespace.
+ Shadowing these namespaces in this way can lead to confusion and unexpected behavior.
+ Code that intends to reference a System or Schema class, enum, or interface may inadvertently reference the locally defined type instead.
+ This can result in ambiguous code and unexpected runtime behavior.
+ It is best to avoid naming your types the same as those in the System or Schema namespace to prevent these issues.
Note that the list of classes, enums, and interfaces in the System and Schema namespaces are determined through
+ io.github.apex-dev-tools:standard-types. It is based on the contents of
+ Salesforce's Apex Reference Guide / System Namespace
+ and Apex Reference Guide / Schema Namespace.
+ As Salesforce introduces new types into the System and Schema namespaces, the rule might not always recognize
+ the new types and produce false-negatives und the standard types are updated.
// Violation: Causes a collision with the `System.Database` class.
+ public class Database {
+ public static String query() {
+ return 'Hello World';
+ }
+ }{0}' defined but not used
+Detects when a local variable is declared and/or assigned but not used.
+ 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);
+ }Avoid having unused methods since they make understanding and maintaining code harder.
+This rule finds not only unused private methods, but public methods as well, as long as +the class itself is not entirely unused. A class is considered used, if it contains at +least one other method/variable declaration that is used, as shown in the +test project file Foo.cls.
+ApexLink is used to make this possible and this needs
+additional configuration. The environment variable PMD_APEX_ROOT_DIRECTORY needs to be set prior to executing
+PMD. With this variable the root directory of the Salesforce metadata, where sfdx-project.json resides, is
+specified. ApexLink can then load all the classes in the project and figure out, whether a method is used or not.
For an accurate analysis it is important that the PMD_APEX_ROOT_DIRECTORY contains a complete set of metadata that
+may be referenced from the Apex source code, such as Custom Objects, Visualforce Pages, Flows and Labels. The
+PMD_APEX_ROOT_DIRECTORY directory must contain a sfdx-project.json, but metadata may be either in the
+SFDX Source format
+or the older MDAPI format. The packageDirectories entries in sfdx-project.json are used to determine which
+directories to search for metadata, if a .forceignore file is present it will be respected.
If the Apex code references external packages via namespace(s) you should declare these in your sfdx-project.json
+file using the 'plugins' syntax shown in the example below to avoid errors. Here's an example of a
+well-formed sfdx-project.json:
+
{
+ "packageDirectories": [
+ {
+ "path": "src",
+ "default": true
+ }
+ ],
+ "namespace": "my_namespace",
+ "sfdcLoginUrl": "https://login.salesforce.com",
+ "sourceApiVersion": "52.0",
+ "plugins": {
+ "dependencies": [
+ {"namespace": "aa"}
+ ]
+ }
+ } public class Triangle {
+ private Double side1;
+ private Double side2;
+ private Double side3;
+
+ public Triangle(Double side1, Double side2, Double side3) {
+ this.side1 = side1;
+ this.side2 = side2;
+ this.side3 = side3;
+ }
+
+ // Method is not invoked so can be removed
+ public Double area() {
+ return (side1 + side2 + side3)/2;
+ }
+ }Avoid using 'while' statements without using braces to surround the code block. If the code +formatting or indentation is lost then it becomes difficult to separate the code being +controlled from the rest.
+ while (true) // not recommended
+ x++;
+
+ while (true) { // preferred approach
+ x++;
+ }