Code Smells & Refactoring Guide

Code Smells & Refactoring Guide

💡 Principles inspired by "Clean Apex Code" by Pablo Gonzalez. Purchase the book for complete coverage.

Overview

Code smells are patterns that indicate potential problems in code structure. This guide helps identify common smells in Apex and provides refactoring strategies.

1. Long Methods

The Smell

Methods exceeding 20-30 lines, doing too much, hard to test in isolation.

Signs

• Method name is vague (processData, handleStuff)
• Multiple levels of nesting
• Many local variables
• Comments separating "sections" of work

Before

public void processOpportunity(Opportunity opp) {
    // Validate opportunity
    if (opp == null) {
        throw new IllegalArgumentException('Opportunity cannot be null');
    }
    if (opp.AccountId == null) {
        throw new IllegalArgumentException('Account is required');
    }
    if (opp.Amount == null || opp.Amount <= 0) {
        throw new IllegalArgumentException('Valid amount required');
    }

    // Calculate discount
    Account acc = [SELECT Type, AnnualRevenue FROM Account WHERE Id = :opp.AccountId];
    Decimal discountRate = 0;
    if (acc.Type == 'Enterprise') {
        if (acc.AnnualRevenue > 1000000) {
            discountRate = 0.20;
        } else {
            discountRate = 0.15;
        }
    } else if (acc.Type == 'Partner') {
        discountRate = 0.10;
    }
    opp.Discount__c = opp.Amount * discountRate;

    // Assign to team
    if (opp.Amount > 100000) {
        opp.OwnerId = getEnterpriseTeamQueue();
    } else if (opp.Amount > 25000) {
        opp.OwnerId = getMidMarketQueue();
    }

    // Send notifications
    if (discountRate > 0.15) {
        sendApprovalRequest(opp);
    }
    if (opp.Amount > 500000) {
        notifyExecutiveTeam(opp);
    }

    update opp;
}

After

public void processOpportunity(Opportunity opp) {
    validateOpportunity(opp);

    Account account = getAccountForOpportunity(opp);
    applyDiscount(opp, account);
    assignToAppropriateTeam(opp);
    sendRequiredNotifications(opp);

    update opp;
}

private void validateOpportunity(Opportunity opp) {
    if (opp == null) {
        throw new IllegalArgumentException('Opportunity cannot be null');
    }
    if (opp.AccountId == null) {
        throw new IllegalArgumentException('Account is required');
    }
    if (opp.Amount == null || opp.Amount <= 0) {
        throw new IllegalArgumentException('Valid amount required');
    }
}

private Account getAccountForOpportunity(Opportunity opp) {
    return [SELECT Type, AnnualRevenue FROM Account WHERE Id = :opp.AccountId];
}

private void applyDiscount(Opportunity opp, Account account) {
    Decimal discountRate = DiscountRules.calculateRate(account);
    opp.Discount__c = opp.Amount * discountRate;
}

private void assignToAppropriateTeam(Opportunity opp) {
    opp.OwnerId = TeamAssignment.getQueueForAmount(opp.Amount);
}

private void sendRequiredNotifications(Opportunity opp) {
    NotificationService.sendIfRequired(opp);
}

When to Extract

Extract a method when:

• The code block can be understood independently
• It doesn't require knowledge of the caller's implementation
• It serves a purpose beyond the immediate use case
• You find yourself writing a comment to explain what a section does

2. Mixed Abstraction Levels

The Smell

A method mixing high-level orchestration with low-level implementation details.

Signs

• Business logic alongside HTTP request building
• Validation mixed with string manipulation
• SOQL queries interspersed with notification logic

Before

public void processNewCustomer(Account account) {
    // HIGH-LEVEL: Validation
    validateAccount(account);

    // LOW-LEVEL: String manipulation
    String sanitizedPhone = account.Phone.replaceAll('[^0-9]', '');
    if (sanitizedPhone.length() == 10) {
        sanitizedPhone = '1' + sanitizedPhone;
    }
    account.Phone = '+' + sanitizedPhone;

    // HIGH-LEVEL: Save
    insert account;

    // LOW-LEVEL: HTTP details
    HttpRequest req = new HttpRequest();
    req.setEndpoint('https://api.crm.com/customers');
    req.setMethod('POST');
    req.setHeader('Content-Type', 'application/json');
    req.setBody(JSON.serialize(account));
    Http http = new Http();
    HttpResponse res = http.send(req);

    // HIGH-LEVEL: Notification
    sendWelcomeEmail(account);
}

After

public void processNewCustomer(Account account) {
    validateAccount(account);
    normalizePhoneNumber(account);
    insert account;
    syncToExternalCRM(account);
    sendWelcomeEmail(account);
}

private void normalizePhoneNumber(Account account) {
    if (String.isBlank(account.Phone)) return;

    String digitsOnly = account.Phone.replaceAll('[^0-9]', '');
    if (digitsOnly.length() == 10) {
        digitsOnly = '1' + digitsOnly;
    }
    account.Phone = '+' + digitsOnly;
}

private void syncToExternalCRM(Account account) {
    ExternalCRMService.syncCustomer(account);
}

Principle

Each method should operate at one level of abstraction. High-level methods orchestrate; low-level methods implement.

3. Boolean Parameter Proliferation

The Smell

Methods with multiple boolean parameters that control behavior.

Signs

• Method calls like process(acc, true, false, true)
• Hard to remember which boolean does what
• Many if/else branches based on parameters

Before

public void createCase(
    String subject,
    String description,
    Id accountId,
    Boolean sendEmail,
    Boolean highPriority,
    Boolean assignToQueue,
    String origin
) {
    Case c = new Case(Subject = subject, Description = description);
    if (highPriority) {
        c.Priority = 'High';
    }
    if (assignToQueue) {
        c.OwnerId = getDefaultQueue();
    }
    // ... more conditionals
}

// Caller - which boolean is which?
createCase('Issue', 'Desc', accId, true, false, true, 'Web');

After: Options Object Pattern

public class CaseOptions {
    public Boolean sendEmail = false;
    public Boolean highPriority = false;
    public Boolean assignToQueue = true;
    public String origin = 'Web';

    public CaseOptions withEmail() {
        this.sendEmail = true;
        return this;
    }

    public CaseOptions withHighPriority() {
        this.highPriority = true;
        return this;
    }

    public CaseOptions withOrigin(String origin) {
        this.origin = origin;
        return this;
    }
}

public Case createCase(String subject, String description, Id accountId, CaseOptions options) {
    Case c = new Case(
        Subject = subject,
        Description = description,
        AccountId = accountId,
        Priority = options.highPriority ? 'High' : 'Medium',
        Origin = options.origin
    );

    if (options.assignToQueue) {
        c.OwnerId = getDefaultQueue();
    }

    insert c;

    if (options.sendEmail) {
        sendCaseConfirmation(c);
    }

    return c;
}

// Clear, self-documenting caller
Case newCase = createCase(
    'Login Issue',
    'Cannot access account',
    accountId,
    new CaseOptions()
        .withEmail()
        .withHighPriority()
        .withOrigin('Phone')
);

4. Magic Numbers and Strings

The Smell

Hardcoded values scattered throughout code without explanation.

Signs

• Numbers like 5, 100, 1000000 without context
• String literals like 'Enterprise', 'Active'
• Same value repeated in multiple places

Before

if (account.AnnualRevenue > 1000000) {
    if (retryCount < 5) {
        if (account.Type == 'Enterprise') {
            // process
        }
    }
}

// Elsewhere in code
if (customer.Revenue__c > 1000000) { }  // Same threshold, different field

After

public class AccountConstants {
    public static final Decimal HIGH_VALUE_THRESHOLD = 1000000;
    public static final Integer MAX_RETRY_ATTEMPTS = 5;
    public static final String TYPE_ENTERPRISE = 'Enterprise';
    public static final String TYPE_PARTNER = 'Partner';
    public static final String STATUS_ACTIVE = 'Active';
}

// Usage
if (account.AnnualRevenue > AccountConstants.HIGH_VALUE_THRESHOLD) {
    if (retryCount < AccountConstants.MAX_RETRY_ATTEMPTS) {
        if (account.Type == AccountConstants.TYPE_ENTERPRISE) {
            // process
        }
    }
}

Benefits

• Single source of truth
• Self-documenting code
• Easy to change values globally
• Prevents typos in string literals

5. Complex Conditionals

The Smell

Long, nested boolean expressions that are hard to understand.

Signs

• Multiple && and || in one expression
• Negations of negations
• Conditions spanning multiple lines without names

Before

if (account.Type == 'Enterprise' &&
    account.AnnualRevenue > 1000000 &&
    account.NumberOfEmployees > 500 &&
    (account.Industry == 'Technology' || account.Industry == 'Finance') &&
    account.BillingCountry == 'United States' &&
    account.Rating == 'Hot') {
    // 50 lines of logic
}

After: Named Boolean Variables

Boolean isEnterpriseCustomer = account.Type == 'Enterprise';
Boolean isHighValue = account.AnnualRevenue > 1000000;
Boolean isLargeCompany = account.NumberOfEmployees > 500;
Boolean isTargetIndustry = account.Industry == 'Technology' ||
                           account.Industry == 'Finance';
Boolean isDomestic = account.BillingCountry == 'United States';
Boolean isHotLead = account.Rating == 'Hot';

Boolean isStrategicAccount = isEnterpriseCustomer &&
                              isHighValue &&
                              isLargeCompany &&
                              isTargetIndustry &&
                              isDomestic &&
                              isHotLead;

if (isStrategicAccount) {
    processStrategicAccount(account);
}

Even Better: Domain Class

// Reusable business rules
public class AccountRules {
    public static Boolean isStrategicAccount(Account account) {
        return isEnterpriseCustomer(account) &&
               isHighValue(account) &&
               isInTargetMarket(account);
    }

    public static Boolean isEnterpriseCustomer(Account account) {
        return account.Type == 'Enterprise' &&
               account.NumberOfEmployees > 500;
    }

    public static Boolean isHighValue(Account account) {
        return account.AnnualRevenue != null &&
               account.AnnualRevenue > 1000000;
    }

    public static Boolean isInTargetMarket(Account account) {
        Set<String> targetIndustries = new Set<String>{'Technology', 'Finance'};
        return targetIndustries.contains(account.Industry) &&
               account.BillingCountry == 'United States';
    }
}

// Clean usage
if (AccountRules.isStrategicAccount(account)) {
    processStrategicAccount(account);
}

6. Duplicate Code

The Smell

Same or similar code repeated in multiple places.

Signs

• Copy-paste patterns
• Same SOQL query in multiple methods
• Similar validation logic across classes

Before

// In AccountService
public void processAccount(Id accountId) {
    Account acc = [
        SELECT Id, Name, Type, Industry, AnnualRevenue, OwnerId
        FROM Account
        WHERE Id = :accountId
    ];
    // process
}

// In ReportingService
public void generateReport(Id accountId) {
    Account acc = [
        SELECT Id, Name, Type, Industry, AnnualRevenue, OwnerId
        FROM Account
        WHERE Id = :accountId
    ];
    // generate
}

// In IntegrationService
public void syncAccount(Id accountId) {
    Account acc = [
        SELECT Id, Name, Type, Industry, AnnualRevenue, OwnerId
        FROM Account
        WHERE Id = :accountId
    ];
    // sync
}

After: Selector Pattern

public class AccountSelector {
    private static final Set<String> STANDARD_FIELDS = new Set<String>{
        'Id', 'Name', 'Type', 'Industry', 'AnnualRevenue', 'OwnerId'
    };

    public Account selectById(Id accountId) {
        List<Account> accounts = selectByIds(new Set<Id>{accountId});
        return accounts.isEmpty() ? null : accounts[0];
    }

    public List<Account> selectByIds(Set<Id> accountIds) {
        if (accountIds == null || accountIds.isEmpty()) {
            return new List<Account>();
        }
        return [
            SELECT Id, Name, Type, Industry, AnnualRevenue, OwnerId
            FROM Account
            WHERE Id IN :accountIds
        ];
    }
}

// All services use the selector
AccountSelector selector = new AccountSelector();
Account acc = selector.selectById(accountId);

7. Deep Nesting

The Smell

Code with many levels of indentation, often from nested if statements.

Signs

• 4+ levels of nesting
• Business logic hidden deep in conditionals
• Hard to follow execution flow

Solution: Guard Clauses

See Best Practices: Guard Clauses for detailed refactoring patterns.

8. God Class

The Smell

A class that knows too much or does too much.

Signs

• Hundreds or thousands of lines
• Many unrelated methods
• Class name is vague (Utility, Helper, Manager)
• Changes to any feature require modifying this class

Solution

Split by responsibility:

• Extract domain classes for business rules
• Create selectors for data access
• Separate services for different business operations
• Use interfaces for dependency injection

See Design Patterns: Domain Class Pattern for implementation guidance.

Refactoring Decision Guide

When NOT to Refactor

• Working code under deadline: Don't refactor what you don't have time to test
• No tests exist: Write tests first, then refactor
• Code is being deprecated: Don't polish what's being removed
• Premature abstraction: Wait until you have 3 concrete examples before abstracting

Testing After Refactoring

Every refactoring should be validated:

1. Run existing tests - They should still pass
2. Check code coverage - Coverage shouldn't drop
3. Verify behavior - Same inputs produce same outputs
4. Performance check - No significant degradation

If tests fail after refactoring that shouldn't change behavior, you've introduced a bug, not just refactored.