Apex Anti-Patterns

Apex Anti-Patterns

Comprehensive catalog of common Apex anti-patterns, code smells, and how to fix them.

Table of Contents

1. Critical Anti-Patterns
2. Code Review Red Flags
3. Performance Anti-Patterns
4. Security Anti-Patterns
5. Testing Anti-Patterns
6. Code Smell Catalog

Critical Anti-Patterns

These patterns will cause immediate failures or security vulnerabilities. NEVER allow these in production code.

1. SOQL in Loop

Problem: Hits 100 SOQL query limit.

❌ BAD:

for (Account acc : accounts) {
    List<Contact> contacts = [SELECT Id FROM Contact WHERE AccountId = :acc.Id];
    // Process contacts
}
// Fails after 100 accounts

✅ GOOD:

Set<Id> accountIds = new Set<Id>();
for (Account acc : accounts) {
    accountIds.add(acc.Id);
}

Map<Id, List<Contact>> contactsByAccountId = new Map<Id, List<Contact>>();
for (Contact con : [SELECT Id, AccountId FROM Contact WHERE AccountId IN :accountIds]) {
    if (!contactsByAccountId.containsKey(con.AccountId)) {
        contactsByAccountId.put(con.AccountId, new List<Contact>());
    }
    contactsByAccountId.get(con.AccountId).add(con);
}

for (Account acc : accounts) {
    List<Contact> contacts = contactsByAccountId.get(acc.Id) ?? new List<Contact>();
    // Process contacts
}

Detection: Search for [SELECT inside for loops.

2. DML in Loop

Problem: Hits 150 DML statement limit.

❌ BAD:

for (Account acc : accounts) {
    acc.Industry = 'Technology';
    update acc;  // DML in loop!
}
// Fails after 150 accounts

✅ GOOD:

for (Account acc : accounts) {
    acc.Industry = 'Technology';
}
update accounts;  // Single DML after loop

Detection: Search for insert, update, delete, upsert inside for loops.

3. Missing Sharing Keyword

Problem: Bypasses record-level security by default.

❌ BAD:

public class AccountService {
    // Implicitly "without sharing" - security risk!
}

✅ GOOD:

public with sharing class AccountService {
    // Respects sharing rules
}

Detection: Classes without with sharing, without sharing, or inherited sharing.

4. Hardcoded Record IDs

Problem: IDs differ between orgs, causing deployment failures.

❌ BAD:

Id recordTypeId = '012000000000000AAA';  // Hardcoded ID!

✅ GOOD:

// Option 1: Query at runtime
Id recordTypeId = Schema.SObjectType.Account.getRecordTypeInfosByDeveloperName()
    .get('Enterprise').getRecordTypeId();

// Option 2: Custom Metadata
Account_Config__mdt config = Account_Config__mdt.getInstance('Default');
Id recordTypeId = config.Record_Type_Id__c;

Detection: 15 or 18-character ID literals in code.

5. Empty Catch Blocks

Problem: Silently swallows errors, making debugging impossible.

❌ BAD:

try {
    insert accounts;
} catch (DmlException e) {
    // Silent failure - no logging!
}

✅ GOOD:

try {
    insert accounts;
} catch (DmlException e) {
    System.debug(LoggingLevel.ERROR, 'Failed to insert accounts: ' + e.getMessage());
    throw e;  // Or handle gracefully with user feedback
}

Detection: catch blocks with no statements or only comments.

6. SOQL Injection

Problem: User input concatenated into SOQL allows malicious queries.

❌ BAD:

String query = 'SELECT Id FROM Account WHERE Name = '' + userInput + ''';
List<Account> accounts = Database.query(query);
// userInput = "test' OR '1'='1" returns ALL accounts!

✅ GOOD:

// Use bind variables
List<Account> accounts = [SELECT Id FROM Account WHERE Name = :userInput];

Detection: String concatenation in SOQL with variables.

7. Test Without Assertions

Problem: False positive tests that pass even when code fails.

❌ BAD:

@IsTest
static void testAccountCreation() {
    Account acc = new Account(Name = 'Test');
    insert acc;
    // No assertions - test passes even if logic is broken!
}

✅ GOOD:

@IsTest
static void testAccountCreation() {
    Account acc = new Account(Name = 'Test', Industry = 'Tech');
    insert acc;

    Account inserted = [SELECT Id, Industry FROM Account WHERE Id = :acc.Id];
    Assert.areEqual('Tech', inserted.Industry, 'Industry should be set');
}

Detection: @IsTest methods with no Assert.* calls.

Code Review Red Flags

These patterns indicate poor code quality and should be refactored.

SOQL Without WHERE or LIMIT

❌ BAD:

List<Account> accounts = [SELECT Id FROM Account];
// Returns ALL accounts - could be millions!

✅ GOOD:

// Option 1: Filter
List<Account> accounts = [SELECT Id FROM Account WHERE Industry = 'Technology'];

// Option 2: Limit
List<Account> accounts = [SELECT Id FROM Account ORDER BY CreatedDate DESC LIMIT 200];

// Option 3: Both
List<Account> accounts = [SELECT Id FROM Account WHERE CreatedDate = THIS_YEAR LIMIT 1000];

Multiple Triggers on Same Object

❌ BAD:

// AccountTrigger1.trigger
trigger AccountTrigger1 on Account (before insert) {
    // Some logic
}

// AccountTrigger2.trigger
trigger AccountTrigger2 on Account (before insert) {
    // More logic - which runs first?
}

✅ GOOD:

// Single trigger + TAF
trigger AccountTrigger on Account (before insert, after insert, before update, after update) {
    new MetadataTriggerHandler().run();
}

// Separate action classes
public class TA_Account_SetDefaults implements TriggerAction.BeforeInsert { }
public class TA_Account_Validate implements TriggerAction.BeforeInsert { }

Generic Exception Only

❌ BAD:

try {
    insert accounts;
} catch (Exception e) {
    // Catches EVERYTHING - too broad
}

✅ GOOD:

try {
    insert accounts;
} catch (DmlException e) {
    // Handle DML errors specifically
    System.debug('DML failed: ' + e.getDmlMessage(0));
} catch (Exception e) {
    // Catch unexpected errors
    System.debug('Unexpected error: ' + e.getMessage());
    throw e;
}

Unnecessary isEmpty() Before DML

❌ BAD:

if (!accounts.isEmpty()) {
    update accounts;
}
// Wastes CPU checking - DML already handles empty lists

✅ GOOD:

update accounts;  // No-op if empty, no error thrown

Double Negative Comparisons

❌ BAD:

if (acc.IsActive__c != false) {
    // Confusing logic
}

✅ GOOD:

if (acc.IsActive__c == true) {
    // Clear intent
}

// Or even better
if (acc.IsActive__c) {
    // Most concise
}

Performance Anti-Patterns

1. Nested Loops with SOQL

❌ BAD:

for (Account acc : accounts) {
    for (Contact con : [SELECT Id FROM Contact WHERE AccountId = :acc.Id]) {
        // Nested SOQL - quadratic complexity!
    }
}

✅ GOOD:

Map<Id, Account> accountsWithContacts = new Map<Id, Account>([
    SELECT Id, (SELECT Id FROM Contacts)
    FROM Account
    WHERE Id IN :accountIds
]);

for (Account acc : accountsWithContacts.values()) {
    for (Contact con : acc.Contacts) {
        // No SOQL in loop
    }
}

2. Querying in Constructor

❌ BAD:

public class AccountService {
    private List<Account> accounts;

    public AccountService() {
        accounts = [SELECT Id FROM Account];  // Runs on EVERY instantiation
    }
}

✅ GOOD:

public class AccountService {
    private List<Account> accounts;

    public AccountService(List<Account> accounts) {
        this.accounts = accounts;  // Inject dependencies
    }

    // Or lazy load only when needed
    private List<Account> getAccounts() {
        if (accounts == null) {
            accounts = [SELECT Id FROM Account LIMIT 200];
        }
        return accounts;
    }
}

3. Excessive CPU Time

❌ BAD:

for (Account acc : accounts) {
    for (Integer i = 0; i < 10000; i++) {
        String hash = EncodingUtil.convertToHex(Crypto.generateDigest('SHA256', Blob.valueOf(acc.Name + i)));
        // Expensive crypto in nested loop
    }
}

✅ GOOD:

// Move expensive operations outside loops
String baseHash = EncodingUtil.convertToHex(Crypto.generateDigest('SHA256', Blob.valueOf('base')));

for (Account acc : accounts) {
    acc.Hash__c = baseHash;  // Reuse computed value
}

4. Inefficient Collections

❌ BAD:

List<Id> uniqueIds = new List<Id>();
for (Id accountId : allIds) {
    if (!uniqueIds.contains(accountId)) {  // O(n) lookup in List
        uniqueIds.add(accountId);
    }
}

✅ GOOD:

Set<Id> uniqueIds = new Set<Id>(allIds);  // O(1) deduplication

Security Anti-Patterns

1. without sharing Everywhere

❌ BAD:

public without sharing class AccountController {
    @AuraEnabled
    public static List<Account> getAccounts() {
        // Bypasses sharing - user sees ALL accounts!
        return [SELECT Id FROM Account];
    }
}

✅ GOOD:

public with sharing class AccountController {
    @AuraEnabled
    public static List<Account> getAccounts() {
        // Respects sharing rules
        return [SELECT Id FROM Account WITH USER_MODE];
    }
}

2. No CRUD/FLS Checks

❌ BAD:

public static void updateAccounts(List<Account> accounts) {
    update accounts;  // No permission check!
}

✅ GOOD:

public static void updateAccounts(List<Account> accounts) {
    if (!Schema.sObjectType.Account.isUpdateable()) {
        throw new SecurityException('User cannot update Accounts');
    }

    // Or use WITH USER_MODE in queries
    update accounts;
}

3. Hardcoded Credentials

❌ BAD:

String apiKey = 'sk_live_abc123xyz';  // NEVER hardcode secrets!

✅ GOOD:

// Use Named Credentials
HttpRequest req = new HttpRequest();
req.setEndpoint('callout:MyNamedCredential/api');  // Auth handled by platform

Testing Anti-Patterns

1. @SeeAllData=true

❌ BAD:

@IsTest(SeeAllData=true)
private class AccountServiceTest {
    // Depends on org data - brittle, slow
}

✅ GOOD:

@IsTest
private class AccountServiceTest {
    @TestSetup
    static void setup() {
        TestDataFactory.createAccounts(10);  // Isolated test data
    }
}

2. No Bulk Testing

❌ BAD:

@IsTest
static void testAccountCreation() {
    Account acc = new Account(Name = 'Test');
    insert acc;
    // Only tests 1 record - misses bulkification bugs
}

✅ GOOD:

@IsTest
static void testBulkAccountCreation() {
    List<Account> accounts = new List<Account>();
    for (Integer i = 0; i < 251; i++) {
        accounts.add(new Account(Name = 'Bulk Test ' + i));
    }

    insert accounts;
    Assert.areEqual(251, [SELECT COUNT() FROM Account]);
}

3. Testing Implementation, Not Behavior

❌ BAD:

@IsTest
static void testGetAccountsCallsQuery() {
    // Tests internal implementation
    Assert.areEqual(1, Limits.getQueries(), 'Should call SOQL once');
}

✅ GOOD:

@IsTest
static void testGetAccountsReturnsCorrectRecords() {
    TestDataFactory.createAccounts(5);

    List<Account> results = AccountService.getAccounts();

    Assert.areEqual(5, results.size(), 'Should return all accounts');
}

Code Smell Catalog

Based on "Clean Apex Code" by Pablo Gonzalez and clean code principles.

Long Method

Smell: Method exceeds 30 lines.

❌ BAD:

public static void processAccount(Account acc) {
    // 100 lines of mixed logic
    if (acc.Industry == 'Tech') {
        // Validation logic
        if (acc.AnnualRevenue == null) { ... }
        // Calculation logic
        Decimal score = ...;
        // DML logic
        update acc;
        // Notification logic
        EmailService.send(...);
    }
}

✅ GOOD:

public static void processAccount(Account acc) {
    validateAccount(acc);
    calculateScore(acc);
    saveAccount(acc);
    notifyOwner(acc);
}

private static void validateAccount(Account acc) { ... }
private static void calculateScore(Account acc) { ... }
private static void saveAccount(Account acc) { ... }
private static void notifyOwner(Account acc) { ... }

Refactoring: Extract Method - split into smaller methods with single responsibilities.

Large Class (God Class)

Smell: Class exceeds 500 lines or has 20+ methods.

❌ BAD:

public class AccountService {
    // 50 methods mixing concerns:
    public static void createAccount() { }
    public static void updateAccount() { }
    public static void validateAccount() { }
    public static void calculateScore() { }
    public static void sendEmail() { }
    public static void generateReport() { }
    // ... 44 more methods
}

✅ GOOD:

// Split by responsibility
public class AccountService { }         // Business logic
public class AccountValidator { }       // Validation
public class AccountScoreCalculator { } // Scoring
public class AccountEmailService { }    // Notifications
public class AccountReportGenerator { } // Reporting

Refactoring: Extract Class - split into multiple classes by concern.

Magic Numbers

Smell: Unexplained numeric literals.

❌ BAD:

if (acc.Score__c > 75) {
    acc.Rating = 'Hot';
}

✅ GOOD:

private static final Integer HOT_LEAD_THRESHOLD = 75;

if (acc.Score__c > HOT_LEAD_THRESHOLD) {
    acc.Rating = 'Hot';
}

Long Parameter List

Smell: Method has 5+ parameters.

❌ BAD:

public static void createAccount(
    String name,
    String industry,
    Decimal revenue,
    String phone,
    String email,
    String website,
    Id ownerId
) { }

✅ GOOD:

public class AccountRequest {
    public String name;
    public String industry;
    public Decimal revenue;
    public String phone;
    public String email;
    public String website;
    public Id ownerId;
}

public static void createAccount(AccountRequest request) { }

Feature Envy

Smell: Method uses more methods/fields from another class than its own.

❌ BAD:

public class OrderService {
    public static Decimal calculateDiscount(Order__c order) {
        Account acc = [SELECT Id, Tier__c FROM Account WHERE Id = :order.Account__c];
        if (acc.Tier__c == 'Gold') {
            return order.Amount__c * 0.2;
        } else if (acc.Tier__c == 'Silver') {
            return order.Amount__c * 0.1;
        }
        return 0;
    }
}

✅ GOOD:

public class Account extends SObject {
    public Decimal getDiscountRate() {
        if (this.Tier__c == 'Gold') return 0.2;
        if (this.Tier__c == 'Silver') return 0.1;
        return 0;
    }
}

public class OrderService {
    public static Decimal calculateDiscount(Order__c order) {
        Account acc = [SELECT Id, Tier__c FROM Account WHERE Id = :order.Account__c];
        return order.Amount__c * acc.getDiscountRate();
    }
}

Refactoring: Move Method - move logic to the class it uses most.

Primitive Obsession

Smell: Using primitives instead of small objects to represent concepts.

❌ BAD:

public static void sendEmail(String address, String subject, String body) {
    // Validates email format inline
    if (!address.contains('@')) throw new InvalidEmailException();
}

✅ GOOD:

public class EmailAddress {
    private String value;

    public EmailAddress(String address) {
        if (!address.contains('@')) {
            throw new InvalidEmailException('Invalid email format');
        }
        this.value = address;
    }

    public String getValue() {
        return value;
    }
}

public static void sendEmail(EmailAddress address, String subject, String body) {
    // Email is already validated
}

Detection Tools

How to find anti-patterns:

VS Code Command:

sf code-analyzer run --workspace force-app/main/default/classes --output-format table

Example output:

Severity  File                    Line  Rule                     Message
────────────────────────────────────────────────────────────────────────────
3         AccountService.cls      45    ApexSOQLInjection        SOQL injection risk
2         AccountTrigger.trigger  12    ApexCRUDViolation        Missing FLS check
1         ContactController.cls   28    ApexUnitTestClassShouldHaveAsserts  No assertions

Refactoring Checklist

When reviewing code, check for:

Bulkification:

• No SOQL in loops
• No DML in loops
• Collections used efficiently (Maps for lookups)
• Tested with 251+ records

Security:

• All classes have sharing keyword
• SOQL uses WITH USER_MODE or Security.stripInaccessible()
• No hardcoded credentials
• No SOQL injection vulnerabilities

Clean Code:

• Methods under 30 lines
• Classes under 500 lines
• No magic numbers (use constants)
• Meaningful variable/method names

Testing:

• All methods covered by tests
• Tests have assertions
• Bulk tests exist (251+ records)
• No @SeeAllData=true

Error Handling:

• No empty catch blocks
• Specific exceptions before generic
• Errors logged with context

Reference

Full Documentation: See docs/ folder for comprehensive guides:

• code-smells-guide.md - Complete code smell catalog
• best-practices.md - Correct patterns
• code-review-checklist.md - 150-point scoring

Back to Main: SKILL.md