salesforce-apex-quality

Salesforce Apex Quality Guardrails

Apply these checks to every Apex class, trigger, and test file you write or review. When reviewing, report findings and propose changes. Edit files only within the scope the user requested, or after the user confirms.

Step 1 — Governor Limit Safety Check

Scan for these patterns before declaring any Apex file acceptable:

SOQL and DML in Loops — Automatic Fail

// ❌ NEVER — causes LimitException at scale
for (Account a : accounts) {
    List<Contact> contacts = [SELECT Id FROM Contact WHERE AccountId = :a.Id]; // SOQL in loop
    update a; // DML in loop
}

// ✅ ALWAYS — collect, then query/update once
Set<Id> accountIds = new Map<Id, Account>(accounts).keySet();
Map<Id, List<Contact>> contactsByAccount = new Map<Id, List<Contact>>();
for (Contact c : [SELECT Id, AccountId FROM Contact WHERE AccountId IN :accountIds]) {
    if (!contactsByAccount.containsKey(c.AccountId)) {
        contactsByAccount.put(c.AccountId, new List<Contact>());
    }
    contactsByAccount.get(c.AccountId).add(c);
}
update accounts; // DML once, outside the loop

Rule: if you see [SELECT or Database.query, insert, update, delete, upsert, merge inside a for loop body — stop and refactor before proceeding.

Step 2 — Sharing Model Verification

Every class must declare its sharing intent explicitly. Undeclared sharing runs in the caller's mode when called from another class, but runs without sharing when the class is the entry point of the transaction (for example a UI controller, REST endpoint, or trigger). Treat an undeclared entry point class as a security finding, and prefer inherited sharing over leaving out a declaration for framework and utility classes.

If a class does not have one of these three declarations, when writing code, add it before writing anything else; when reviewing, report it as a finding.

Step 3 — CRUD / FLS Enforcement

Apex code that reads or writes records on behalf of a user must verify object and field access. The platform does not enforce FLS or CRUD automatically in Apex.

// Check before querying a field
if (!Schema.sObjectType.Contact.fields.Email.isAccessible()) {
    throw new System.NoAccessException();
}

// Or use WITH USER_MODE in SOQL (API 56.0+)
List<Contact> contacts = [SELECT Id, Email FROM Contact WHERE AccountId = :accId WITH USER_MODE];

// Or use Database.query with AccessLevel
List<Contact> contacts = Database.query('SELECT Id, Email FROM Contact', AccessLevel.USER_MODE);

Writes need the same protection. Run DML on behalf of users in user mode (USER_MODE), or remove fields the user cannot create or update before the write, and check that the user can create, update, or delete the object before the matching DML.

Rule: any Apex method callable from a UI component, REST endpoint, or @InvocableMethod must enforce CRUD/FLS. Sharing declarations such as with sharing control record visibility only and do not replace CRUD/FLS checks. Internal service methods called only from trusted contexts may leave out CRUD/FLS checks only with a documented reason in a code comment.

Step 4 — SOQL Injection Prevention

// ❌ NEVER — concatenates user input into SOQL string
String soql = 'SELECT Id FROM Account WHERE Name = \'' + userInput + '\'';

// ✅ ALWAYS — bind variable
List<Account> accounts = [SELECT Id FROM Account WHERE Name = :userInput];

// ✅ For dynamic SOQL with user-controlled field names — validate against a whitelist
Set<String> allowedFields = new Set<String>{'Name', 'Industry', 'AnnualRevenue'};
if (!allowedFields.contains(userInput)) {
    throw new IllegalArgumentException('Field not permitted: ' + userInput);
}

For dynamic SOQL, pass user supplied values only as bind variables and run the query in user mode; escaping quotes is only a fallback. Add a field name to the query string only after it passes the allowlist check, ideally one built from Schema describe results.

Step 5 — Modern Apex Idioms

Prefer current language features. Each idiom has its own minimum API version, so check the project's source API version before applying one:

Step 6 — PNB Test Coverage Checklist

Every feature must be tested across all three paths. Missing any one of these is a quality failure:

Positive Path

• Expected input → expected output.
• Assert the exact field values, record counts, or return values — not just that no exception was thrown.

Negative Path

• Invalid input, null values, empty collections, and error conditions.
• Assert that exceptions are thrown with the correct type and message.
• Assert that no records were mutated when the operation should have failed cleanly.

Bulk Path

• Insert/update/delete 200–251 records in a single test transaction.
• Assert that all records processed correctly — no partial failures from governor limits.
• Wrap the code under test in Test.startTest() / Test.stopTest(): startTest gives the code under test a fresh set of governor limits, and stopTest makes queued async work run before your assertions.

Test Class Rules

@isTest(SeeAllData=false)   // Required — no exceptions without a documented reason
private class AccountServiceTest {

    @TestSetup
    static void makeData() {
        // Create all test data here — use a factory if one exists in the project
    }

    @isTest
    static void givenValidInput_whenProcessAccounts_thenFieldsUpdated() {
        // Positive path
        List<Account> accounts = [SELECT Id FROM Account LIMIT 10];
        Test.startTest();
        AccountService.processAccounts(accounts);
        Test.stopTest();
        // Assert meaningful outcomes — not just no exception
        List<Account> updated = [SELECT Status__c FROM Account WHERE Id IN :accounts];
        Assert.areEqual('Processed', updated[0].Status__c, 'Status should be Processed');
    }
}

Step 7 — Trigger Architecture Checklist

• One trigger per object. If a second trigger exists, consolidate into the handler.
• Trigger body contains only: context checks, handler invocation, and routing logic.
• No business logic, SOQL, or DML directly in the trigger body.
• If a trigger framework (Trigger Actions Framework, ff-apex-common, custom base class) is already in use — extend it. Do not create a parallel pattern.
• Handler class is with sharing unless the trigger requires elevated access. In that case, keep the elevated logic in a minimal helper class with a documented reason.

Quick Reference — Hardcoded Anti-Patterns Summary