Principles of Code Review: Understanding Readability

I’m exploring a framework to define code readability by balancing locality and cyclomatic complexity.

In recent months, I have worked to define what programmers mean when we say "code is readable." Through experience, many of us develop an intuitive sense of readability, but this understanding often remains personal and non-transferrable. "Readable code" frequently translates to "code I understand," which creates two problems: First, code that was once unreadable becomes readable to an individual over time, but this personal comprehension fails to serve the broader team. Second, as programming idioms evolve, previously readable code can become difficult to understand.

To address these challenges, I have developed a framework for defining code readability independent of personal intuition. This framework remains a work in progress, and I welcome both positive feedback and constructive criticism. The article explores multiple tensions that I balance when writing code, beginning with how locality of behavior¹ interacts with cyclomatic complexity².

Understanding Tension as a Framework

The most effective way to establish guidelines for subjective concepts is to identify opposing beneficial principles and recognize their inherent tension. This approach allows teams to use their collective experience to determine where specific code should fall along this spectrum.

While few absolute rules exist in defining "good" code, cyclomatic complexity less than 15 stands as a rare exception—complexity above this threshold consistently indicates problematic code. My approach to evaluating and improving code involves identifying two principles in tension, establishing their range, and then positioning the code under review within that spectrum.

Core Principles

Locality of Behavior (LoB)

The behavior of a code unit should be immediately apparent by examining only that unit of code. This principle emphasizes self-contained, self-explanatory code segments.

Cyclomatic Complexity

Defined as the minimum number of test cases needed for complete coverage of a code unit, cyclomatic complexity serves as a predictor of future maintenance requirements. It can be approximated using the following formula:

While the NIST guidelines recommend maintaining complexity below 10, I advocate for a maximum of 5 to ensure maintainability.

The Tension: LoB vs. Cyclomatic Complexity

Functions and design patterns serve as primary tools for managing cyclomatic complexity. Functions encapsulate behavior or transformation units, while design patterns provide recognizable structures for common programming scenarios. For instance, the builder pattern can streamline code blocks, and the observer pattern effectively decouples concerns.

Functions particularly excel at reducing cyclomatic complexity. Moving a block of if/else statements into a named function can make considerable reductions in local complexity. However, this approach relocates knowledge away from its immediate context. As codebases grow, functions that were once "nearby" tend to drift apart, reducing code locality while benefiting local complexity.

Balancing the Trade-offs

When cyclomatic complexity exceeds 10, locality of behavior becomes a secondary concern—the complexity has reached a level where local understanding is compromised regardless of location. The solution is to refactor until complexity falls below this threshold. After such refactoring, previously obscured behaviors often become apparent, revealing opportunities for further separation through design patterns or data-driven approaches.

Practical Example: Client Side Notification

Let's examine a real-world scenario involving a notification system. Consider server messages that must be translated into visual elements for display.

Initial Implementation

function translateEvent(notification: Notification): string {
    if(notification.event == UserInteractionEvent.LIKE) {
        return "someone liked your comment";
    } else {
        return "someone commented on your post";
    }
}

This implementation has a cyclomatic complexity of 2, requiring two unit tests for complete coverage.

Evolution: Growing Requirements and Emerging Tensions

The clearest demonstration of tension between Locality of Behavior (LoB) and cyclomatic complexity emerges as we expand our notification system's requirements. Let's examine this as a naturally growing application would experience.

Stage 1: Adding New Event Types

Initially, our system needs to handle additional event types: replies and superlikes. This first expansion reveals how quickly complexity can grow even with seemingly simple additions.

// Original version - Complexity: 2
function translateEvent(notification: Notification): string {
    if(notification.event == UserInteractionEvent.LIKE) {
        return "someone liked your comment";
    } else {
        return "someone commented on your post";
    }
}

// First attempt at expansion - Complexity: 4
function translateEvent(notification: Notification): string {
    if(notification.event == UserInteractionEvent.LIKE) {
        return "someone liked your comment";
    } else if(notification.event == UserInteractionEvent.COMMENT) {
        return "someone commented on your post";
    } else if(notification.event == UserInteractionEvent.REPLY) {
        return "someone replied to your post";
    } else {
        return "someone super-liked your post";
    }
}

This straightforward, expansion immediately reveals several issues:

1. The cyclomatic complexity doubles (but is still under our soft guideline of 5).
2. The code structure becomes harder to scan.
3. Adding new events requires carefully managing the else-if chain.
4. There's no compile-time guarantee we're handling all cases.
5. The else case makes an assumption that is only true by coincidence, and will likely stop being true quite soon.

Stage 2: Implementing Type Safety

To address these concerns, we can refactor to a switch statement with exhaustive type checking:

function assertUnreachable(_: never): never {
    throw new Error("Didn't expect to get here");
}

function translateEvent(notification: Notification): string {
    switch(notification.event) {
        case UserInteractionEvent.LIKE:
            return "someone liked your comment";
        case UserInteractionEvent.COMMENT:
            return "someone commented on your post";
        case UserInteractionEvent.REPLY:
            return "someone replied to your post";
        case UserInteractionEvent.SUPERLIKE:
            return "someone super-liked your post";
        default: 
            return assertUnreachable(notification.event);
    }
}

This refactoring brings several benefits:

1. Maintains the same cyclomatic complexity (4).
2. Adds compile-time exhaustiveness checking.
3. Improves code scanability.
4. Makes adding new cases more structured.

However, we're about to see how this structure handles additional complexity poorly when we add user context requirements. Similar exhaustiveness checking can be done in python as well³.

Stage 3: Adding User Context

Now let's add the requirement to show the user's name or "You" for the current user. Our first instinct might be to modify the existing switch statement:

function translateEvent(notification: Notification): string {
    const who = notification.UID == currentUserUID ? 'You' : notification.name;

    switch(notification.event) {
        case UserInteractionEvent.LIKE:
            return `${who} liked your comment`;
        case UserInteractionEvent.COMMENT:
            return `${who} commented on your post`;
        case UserInteractionEvent.REPLY:
            return `${who} replied to your post`;
        case UserInteractionEvent.SUPERLIKE:
            return `${who} super-liked your post`;
        default: 
            return assertUnreachable(notification.event);
    }
}

While this works, it introduces several problems:

1. The who logic is counted in the complexity of translateEvent.
2. Future modifications to user display logic may require changes in multiple places

Stage 4: Separating Concerns

We can improve this by extracting the user name logic:

const translateName = (notification: Notification) => 
    notification.UID == currentUserUID ? 'You' : notification.name;

function translateEvent(notification: Notification): string {
    const who = translateName(notification);

    switch(notification.event) {
        case UserInteractionEvent.LIKE:
            return `${who} liked your comment`;
        case UserInteractionEvent.COMMENT:
            return `${who} commented on your post`;
        case UserInteractionEvent.REPLY:
            return `${who} replied to your post`;
        case UserInteractionEvent.SUPERLIKE:
            return `${who} super-liked your post`;
        default: 
            return assertUnreachable(notification.event);
    }
}

This refactoring demonstrates the key tension:

1. We've improved maintainability by extracting the name logic.
2. We've reduced the effective complexity of each case
3. However, we've decreased locality - understanding the full behavior now requires reading two functions.
4. The trade-off is worthwhile because it makes the code more resilient to future changes, and more independently testable.

This progression clearly shows how the tensions between LoB and cyclomatic complexity emerge and evolve as requirements grow. Each stage forces us to balance these competing concerns, leading to different architectural decisions.

As systems grow to include numerous event types, the switch statement approach becomes increasingly unwieldy. One powerful alternative is transforming this logic into a data-driven approach, I picked up from embedded systems which I call 'table-driven code.' This transformation not only simplifies the immediate implementation but also opens up strategic possibilities: the logic could be moved to the backend as an API response, or the behavior could be defined in a configuration file.

By converting complex conditional logic into structured data, we maintain high locality of behavior while reducing the code to a simple data transformation. Let's examine how this approach changes our notification system:

const eventTextsSuffixes: [UserInteractionEvent, string][] = [
    [UserInteractionEvent.LIKE, 'liked your comment'],
    [UserInteractionEvent.COMMENT, 'commented on your post'],
    [UserInteractionEvent.REPLY, 'replied on your post'],
    [UserInteractionEvent.SUPERLIKE, 'LOVED IT!!'],
];

const translateName = (notification: Notification) => 
    notification.UID == currentUserUID ? 'You' : notification.name;

function translateEvent(notification: Notification): string {
    const who = translateName(notification);
    const suffix = eventTextsSuffixes.find(([key]) => key == notification.event) 
        ?? [-1, 'unknown event'];
    return `${who} ${suffix[1]}`;
}

This final refactoring achieves a cyclomatic complexity of 2 while maintaining good locality of behavior. However, it trades static type checking for runtime flexibility.

Conclusion

The tension between locality of behavior and cyclomatic complexity exemplifies the trade-offs inherent in software development. While both principles contribute to code quality, their opposition requires careful balance. Through systematic refactoring and thoughtful application of design patterns, we can achieve maintainable code that remains both comprehensible and testable.

The key lessons from this exploration include:

1. Maintain cyclomatic complexity below 10 as a primary goal.
2. Use function extraction and design patterns judiciously.
3. Consider data-driven approaches for repetitive logic.
4. Balance the trade-off between locality and complexity based on your specific context

Remember that these guidelines serve as a framework for discussion rather than rigid rules, allowing teams to make informed decisions about code organization and structure, and refine those decisions over time.