Apex Code Review Checklist: What to Flag on Every Salesforce PR

An Apex PR review that takes 10 minutes can catch 80 percent of the bugs that would otherwise reach production. The remaining 20 percent are subtle architectural issues that need an architect's eye. The 80 percent are mechanical: SOQL in loops, missing sharing keywords, empty test assertions, hardcoded IDs. The pattern is to run a structured checklist on every PR.

Most Salesforce teams either skip code review (small teams, "we trust each other") or do unstructured review (read the diff, leave comments). Both miss the same predictable bugs. The fix is the checklist.

The checklist

Sapota's reviewers run through these categories on every Apex PR. The whole pass takes 8-15 minutes for a typical PR.

1. Governor limits and bulkification

• No SOQL queries inside for loops.
• No DML statements inside for loops.
• No callouts inside for loops.
• Trigger handler has a 200-record bulk test.
• Async methods tested with Test.startTest/Test.stopTest.
• Aggregate queries used where appropriate (instead of querying all records then summing in code).

2. Sharing, CRUD, FLS

• Every class has an explicit sharing mode declaration (with sharing, without sharing, or inherited sharing).
• without sharing usage has a documented justification in code comment.
• SOQL queries on user-facing controllers use WITH USER_MODE or WITH SECURITY_ENFORCED, or results pass through Security.stripInaccessible.
• DML on user-touched records uses Database.insert(records, AccessLevel.USER_MODE) or equivalent.
• @AuraEnabled methods are appropriately restrictive (no broad system access).

3. Trigger architecture

• One trigger per object (object's existing trigger, no new triggers added).
• Trigger file is one line: new ObjectHandler().run();.
• Handler class includes a static recursion guard.
• Logic delegated to service classes when handler method exceeds ~50 lines.

4. Error handling

• Database.insert/update/delete with allOrNone = false inspects SaveResult and logs failures.
• Try/catch blocks log the exception and re-raise or handle explicitly (no silent swallow).
• Custom exceptions used for business-logic failures, not generic Exception.
• Async methods handle their own errors (no propagation to a caller that does not exist).

5. Test quality

• Every test has at least one meaningful System.assertEquals or System.assertNotEquals.
• Tests cover the happy path and at least one edge case (null, empty, boundary, failure).
• Bulk tests for trigger handlers use 200 records.
• Test data created via factory class (TestDataFactory or similar), not inline.
• Async tests use Test.startTest/Test.stopTest discipline.
• Tests do not use seeAllData=true unless absolutely justified.

6. Code quality

• No hardcoded record IDs (use queries or test data factory).
• No hardcoded credentials or endpoints (use Named Credentials).
• Method and variable names are descriptive (no tmp, data1, process).
• No commented-out code blocks left in the PR.
• Apex Doc comments on public class methods.

7. Integration patterns

• Apex callouts have explicit timeouts (not the 10-second default).
• Callouts have retry logic for transient failures, or are explicitly fire-and-forget with documented intent.
• Callouts to non-idempotent endpoints include idempotency keys.
• Callouts after DML are moved to async context (Queueable with Database.AllowsCallouts).

8. Schema and metadata changes

• New custom fields have a clear purpose and naming convention.
• New custom objects are reviewed for master-detail vs lookup decisions.
• New validation rules tested against expected legitimate inputs.
• Field-level security configured for relevant profiles (not just inherited from default).

A worked example

A small PR adds an Apex method that processes uploaded Account records:

public class AccountUploadController {
  @AuraEnabled
  public static String processUpload(List<Account> uploaded) {
    List<Account> toInsert = new List<Account>();
    for (Account a : uploaded) {
      User owner = [SELECT Id, Region__c FROM User
                    WHERE Username = :a.OwnerEmail__c LIMIT 1];
      a.Region__c = owner.Region__c;
      a.OwnerId = owner.Id;
      toInsert.add(a);
      insert toInsert;
    }
    return 'Success';
  }
}

Run the checklist:

• Governor limits and bulkification: ❌ SOQL inside loop (User query). ❌ DML inside loop (insert toInsert). ❌ No bulk test mentioned.
• Sharing, CRUD, FLS: ❌ Class has no sharing declaration. ❌ SOQL has no security enforcement. ❌ DML has no access check.
• Trigger architecture: Not a trigger, N/A.
• Error handling: ❌ No try/catch around DML. Failure throws to LWC without graceful handling. ❌ Returns 'Success' regardless of actual outcome.
• Test quality: PR does not include a test file (would be a separate red flag).
• Code quality: Method name OK. Variable name toInsert OK.

The PR has 6 blockers across 4 categories. The reviewer asks for changes. The author refactors:

public with sharing class AccountUploadController {
  @AuraEnabled
  public static String processUpload(List<Account> uploaded) {
    // Bulkify: collect emails first.
    Set<String> emails = new Set<String>();
    for (Account a : uploaded) {
      if (a.OwnerEmail__c != null) emails.add(a.OwnerEmail__c);
    }

    // Single query.
    Map<String, User> usersByEmail = new Map<String, User>();
    for (User u : [SELECT Id, Username, Region__c
                   FROM User
                   WHERE Username IN :emails
                   WITH USER_MODE]) {
      usersByEmail.put(u.Username, u);
    }

    // Iterate using the map.
    List<Account> toInsert = new List<Account>();
    for (Account a : uploaded) {
      User owner = usersByEmail.get(a.OwnerEmail__c);
      if (owner != null) {
        a.Region__c = owner.Region__c;
        a.OwnerId = owner.Id;
        toInsert.add(a);
      }
    }

    // Single bulk DML with partial-success handling.
    Database.SaveResult[] results = Database.insert(toInsert,
                                                     AccessLevel.USER_MODE);
    Integer failed = 0;
    for (Database.SaveResult sr : results) {
      if (!sr.isSuccess()) failed++;
    }
    return failed > 0 ? 'Partial success: ' + failed + ' failed' : 'Success';
  }
}

Re-run the checklist: all categories pass. The PR is ready to merge after a test class is added.

The before-version would have shipped to production and broken on the 101st Account upload. The reviewer's 10 minutes saved a production incident.

What good code review discipline looks like

A Salesforce team running healthy Apex review:

• The checklist is documented and accessible to every reviewer.
• New reviewers shadow experienced reviewers for the first 5 PRs.
• Authors self-check against the list before requesting review.
• Static analysis (PMD, Salesforce Code Analyzer) runs in CI to catch the mechanical items automatically.
• Architects review architectural decisions; checklist items are below their bar.

The biggest leverage on Apex quality is making the checklist habitual. After a few months, the checks become reflexive. Reviewers spot the patterns without consciously running through the list.

Automating the easy parts

Salesforce Code Analyzer (PMD-based) flags many checklist items automatically:

• SOQL/DML inside loops.
• Methods exceeding complexity thresholds.
• Hardcoded IDs.
• Test classes without assertions.

Running it in CI (GitHub Actions, GitLab CI) on every PR catches the mechanical issues before a human looks. The human review focuses on the items the tool cannot catch: architectural decisions, security context, edge case test coverage.

This is the leverage. The tool does 60 percent of the review. The human does the other 40 percent that requires judgment.

Common review anti-patterns

Five patterns Sapota has seen in audits:

• Diff-only review. Reviewer reads the diff, leaves a few stylistic comments, approves. Misses everything that requires looking outside the diff (sharing context, related code paths).
• Approval shopping. Author requests review from the lightest-touch reviewer. Standards drift over time as the easiest path becomes the default.
• No standard. Different reviewers flag different things. Authors cannot predict what will block their PR.
• Auto-merge on green. CI passes, PR merges. No human looked.
• Reviewer fatigue. Senior engineer is the only effective reviewer; queue piles up; PRs sit for days. Distribute review responsibility through pairing or rotation.

What good review looks like

A Salesforce team with healthy code review:

• Every PR has at least one human reviewer plus CI checks.
• Reviewers use a shared checklist.
• Static analysis catches the mechanical issues automatically.
• Architectural changes get architect review specifically.
• Review turnaround is measured in hours, not days.
• Authors and reviewers see review as a learning loop, not a gate.

Sapota's Salesforce team holds the Platform Developer I credential and runs PR review with the checklist on every engagement. The discipline pays off in the form of governor limit incidents that did not happen, sharing leaks that did not ship, and onboarding curves that are short rather than expensive.

---

Setting up or improving Apex code review discipline? Sapota's Salesforce team handles review process design, static analysis CI integration, and reviewer training on production engagements. Get in touch ->

See our full platform services for the stack we cover.