Maintainability First Code Review

Maintainability-First Code Review

Acts as a strict maintainability gatekeeper during PR or diff reviews, identifying structural problems before they become technical debt. Evaluates code changes for file sprawl, spaghetti conditionals, thin abstractions, and layer violations, then recommends concrete restructurings that simplify the implementation while preserving behavior.

TL;DR Checklist

• Verify no file exceeds ~1000 lines of code
• Verify no function exceeds ~50 lines or nesting depth > 4
• Search for ad-hoc conditionals and scattered special-case branches
• Flag thin wrappers and pass-through abstractions that add indirection without value
• Ensure feature logic does not leak into shared or core modules
• Identify at least one "code judo" restructuring opportunity per review

When to Use

Use this skill when:

• Reviewing a diff or changed files for maintainability violations (file sprawl, spaghetti code, thin abstractions)
• A file or function has grown large or a new layer is being added without justification
• Feature logic appears to be leaking into shared, core, or framework code
• A rewrite is proposed and you need to judge whether it removes complexity or just moves it
• An automated agent wrote code fast and the question is whether anyone can maintain it

When NOT to Use

Avoid this skill for:

• Cosmetic nits (whitespace, naming preferences) — use code-review instead
• Whether the code "works" functionally — use proving-claims or test verification instead
• Trivial, obvious edits with no effect on structure
• Security auditing — use security-review instead
• Strategic architecture planning across multiple modules — use architectural-review instead

Core Workflow

1. Scope Assessment — Read the diff or changed files. Identify file sizes, LOC counts, and which module boundaries are touched. Checkpoint: If no file is > 200 lines and no new abstractions are introduced, the review scope is narrow — proceed efficiently.

2. Code Structure Analysis — Evaluate LOC thresholds. Flag files approaching 1000 lines, functions approaching 50 lines, and nesting exceeding 4 levels. Checkpoint: Record each threshold breach with the specific file and line number.

3. Code Judo Analysis — Identify restructuring opportunities that preserve behavior while simplifying the implementation. Look for flattening nested conditionals, eliminating redundant intermediate steps, extracting complex logic into domain functions, replacing if-else chains with strategy patterns, and removing pass-through wrappers. Checkpoint: For each restructure, verify the new version produces identical behavior.

4. Spaghetti Pattern Detection — Search for ad-hoc conditionals scattered across unrelated flows, feature-specific logic in shared code, silent fallbacks that hide unclear invariants, and type boundary violations (any, unknown without explicit types). Checkpoint: Distinguish between necessary complexity (domain logic) and accidental complexity (poor structure).

5. Layer Integrity Verification — Ensure canonical modules (auth, logging, routing) do not accumulate feature-specific logic. Verify dependencies only exist where they reduce complexity. Checkpoint: Every new dependency or import must justify itself by removing more complexity than it adds.

6. Severity Scoring — Classify findings as warning (maintainability concern), error (structural violation), or block (critical threat). Checkpoint: At least one actionable finding should be reported per review unless the code passes all checks.

7. Deliver Findings — Produce a structured review with: prioritized findings, suggested restructures, and a final verdict (APPROVED, NEEDS_CHANGES, or BLOCKED). Checkpoint: Every finding cites a specific location and a concrete fix, not a general feeling.

Implementation Patterns

Pattern 1: Maintainability Thresholds

Check code against measurable size and complexity thresholds. These are heuristics, not laws — but they should trigger a review whenever violated.

# ✅ GOOD — code structure respects maintainability thresholds
MAX_FILE_LINES = 1000
MAX_FUNCTION_LINES = 50
MAX_NESTING_DEPTH = 4

def assess_file(file_path: str) -> list[str]:
    """Assess a file for maintainability violations.

    Returns a list of violation messages.
    """
    violations: list[str] = []

    with open(file_path) as f:
        lines = f.readlines()

    total_lines = len(lines)

    # File size check
    if total_lines > MAX_FILE_LINES:
        violations.append(
            f"BLOCK: File exceeds {MAX_FILE_LINES} lines "
            f"({total_lines} lines). Split into smaller modules."
        )

    # Function size check (simplified scan)
    for i, line in enumerate(lines, 1):
        if line.strip().startswith("def ") and "(" in line:
            func_start = i
            func_lines = 1
            for j in range(i, min(i + MAX_FUNCTION_LINES + 20, len(lines))):
                next_line = lines[j]
                if next_line.strip() and not next_line.startswith(" ") and not next_line.startswith("\t"):
                    break
                func_lines += 1
            if func_lines > MAX_FUNCTION_LINES:
                violations.append(
                    f"ERROR: Function at line {func_start} exceeds "
                    f"{MAX_FUNCTION_LINES} lines ({func_lines} lines)."
                )

    return violations

Pattern 2: Spaghetti Conditionals Detection

Ad-hoc conditionals inserted into unrelated code paths are a primary source of spaghetti code. They grow incrementally with no architectural justification.

# ❌ BAD — ad-hoc conditional scattered into an unrelated flow
def process_order(order: dict) -> dict:
    """Process an order — grows a conditional every time a new case appears."""
    # Original logic...
    result = calculate_pricing(order)

    # Ad-hoc: "we also need this check now"
    if order.get("channel") == "wholesale" and order.get("priority") == "rush":
        result["discount"] = 0.15
    elif order.get("channel") == "wholesale":
        result["discount"] = 0.10
    elif order.get("channel") == "retail" and order.get("is_vip"):
        result["discount"] = 0.20

    # More ad-hoc branches appear here over time...
    return result

# ✅ GOOD — logic extracted into a dedicated pricing policy
from enum import Enum
from dataclasses import dataclass


class Channel(str, Enum):
    WHOLESALE = "wholesale"
    RETAIL = "retail"
    DIRECT = "direct"


@dataclass
class OrderProfile:
    channel: Channel
    priority: str | None = None
    is_vip: bool = False


class PricingPolicy:
    """Determines discount based on order profile."""

    def get_discount(self, profile: OrderProfile) -> float:
        if profile.channel == Channel.WHOLESALE and profile.priority == "rush":
            return 0.15
        if profile.channel == Channel.WHOLESALE:
            return 0.10
        if profile.channel == Channel.RETAIL and profile.is_vip:
            return 0.20
        return 0.0


def process_order(order: dict) -> dict:
    profile = OrderProfile(
        channel=Channel(order["channel"]),
        priority=order.get("priority"),
        is_vip=order.get("is_vip", False),
    )
    discount = PricingPolicy().get_discount(profile)
    result = calculate_pricing(order)
    result["discount"] = discount
    return result

Pattern 3: Code Judo — Flattening Nested Conditionals

Flatten deeply nested conditionals using guard clauses (early returns). Each nesting level adds cognitive load; reducing depth is the single highest-impact restructuring.

# ❌ BAD — 4 levels of nesting, hard to follow control flow
def create_user_profile(request_data: dict) -> dict | None:
    if request_data.get("email"):
        if "@" in request_data["email"]:
            if len(request_data["email"]) < 256:
                if not is_email_taken(request_data["email"]):
                    profile = {
                        "email": request_data["email"],
                        "name": request_data.get("name", ""),
                    }
                    save_to_db(profile)
                    return profile
                else:
                    return {"error": "Email already taken"}
            else:
                return {"error": "Email too long"}
        else:
            return {"error": "Invalid email format"}
    else:
        return {"error": "Email required"}

# ✅ GOOD — flattened with guard clauses, 0 nesting depth
def create_user_profile(request_data: dict) -> dict:
    email = request_data.get("email")
    if not email:
        return {"error": "Email required"}
    if "@" not in email:
        return {"error": "Invalid email format"}
    if len(email) >= 256:
        return {"error": "Email too long"}
    if is_email_taken(email):
        return {"error": "Email already taken"}

    return {"email": email, "name": request_data.get("name", "")}

Pattern 4: Thin Abstraction Detection

A thin abstraction (pass-through wrapper) adds indirection without introducing meaningful behavior. It obscures the call chain and makes refactoring harder.

# ❌ BAD — thin wrappers that do nothing but forward parameters
class UserService:
    def get_user(self, user_id: int) -> dict:
        return self.repository.find_by_id(user_id)

    def get_user_by_email(self, email: str) -> dict:
        return self.repository.find_by_email(email)

    def create_user(self, data: dict) -> dict:
        return self.repository.insert(data)

    def update_user(self, user_id: int, data: dict) -> dict:
        return self.repository.update(user_id, data)

    def delete_user(self, user_id: int) -> None:
        self.repository.delete(user_id)

# Check if a wrapper is thin:
def is_thin_wrapper(func) -> bool:
    """Detect if a function merely forwards its arguments."""
    import inspect

    source = inspect.getsource(func)
    # Count parameter references in the body
    params = [p.name for p in inspect.signature(func).parameters.values()]
    references = sum(
        1 for param in params
        if param in source and source.count(param) <= 3
    )
    return references >= len(params) and "self.repository" in source

Pattern 5: Layer Integrity — Feature Logic Leakage

Shared or canonical modules must not accumulate feature-specific logic. This is one of the most common forms of structural decay.

# ❌ BAD — feature-specific logic leaked into shared auth module
# File: shared/auth_middleware.py
async def auth_middleware(request: Request) -> dict:
    token = request.headers.get("Authorization")
    user = validate_token(token)

    # Feature-specific: this belongs in the billing module, not here
    if user.get("tier") == "enterprise" and request.url.path.startswith("/api/reports"):
        request.state.allow_report_download = True

    # More feature logic accumulates...
    if user.get("tier") == "premium":
        request.state.rate_limit = 1000
    elif user.get("tier") == "free":
        request.state.rate_limit = 100

    return user

# ✅ GOOD — auth module handles only authentication; feature concerns are delegated
async def auth_middleware(request: Request) -> dict:
    token = request.headers.get("Authorization")
    user = validate_token(token)
    request.state.user = user
    return user


# Feature-specific logic lives in its own module
class AuthorizationPolicy:
    """Determines access based on user tier and requested resource."""

    def check_access(self, user: dict, request: Request) -> dict:
        checks = {
            "enterprise": self._enterprise_checks,
            "premium": self._premium_checks,
            "free": self._free_checks,
        }
        tier = user.get("tier", "free")
        return checks.get(tier, self._free_checks)(request)

    def _enterprise_checks(self, request: Request) -> dict:
        return {"rate_limit": 1000, "allow_report_download": True}

    def _premium_checks(self, request: Request) -> dict:
        return {"rate_limit": 1000}

    def _free_checks(self, request: Request) -> dict:
        return {"rate_limit": 100}

Constraints

MUST DO

• Evaluate file sizes and LOC counts before assessing code quality — size drives structure
• Search for code judo opportunities: find the simplest restructure that preserves behavior
• Treat ad-hoc conditionals as design problems, not stylistic nits — flag them prominently
• Check that new abstractions remove more complexity than they introduce — flag thin pass-throughs
• Verify feature logic does not leak into shared, core, or canonical modules
• Use explicit typed models or contracts at module boundaries instead of loosely-shaped objects
• Report findings with specific file paths and line numbers — never with vague impressions
• Recommend deletion over rearrangement when reducing complexity

MUST NOT DO

• Flag cosmetic issues (whitespace, naming style, formatting) — these are not maintainability concerns
• Accept "it works" as justification for structural debt — functionality is the minimum bar
• Use any or unknown types at boundaries when a clearer type contract exists
• Create pass-through wrappers that merely forward parameters to underlying functions
• Let canonical modules (auth, logging, routing, error handling) accumulate feature-specific logic
• Add silent fallbacks that hide unclear invariants — make boundaries explicit instead
• Flag complexity that represents genuine domain logic as a code quality problem
• Use the 1000-line or 50-line thresholds as absolute laws — they are heuristics that trigger review

Output Template

When applying this skill, produce the following structured review:

1. Summary

• Files reviewed: list of files with line counts
• Total new lines added / removed
• Structural health score: WARMING (minor concerns), HOT (violations present), CRITICAL (structural threats)

2. Findings (Ranked by Severity)

Each finding must include:

• Location: file.py:line_number
• Violation: What maintainability standard is at risk
• Evidence: Brief description of the problem
• Suggested Fix: Concrete restructuring, not a general principle
• Severity: WARN / ERROR / BLOCK

3. Code Judo Opportunities

• At least one specific restructuring suggestion per review
• Show the before/after with minimal diff context
• Explain why the restructure preserves behavior and reduces complexity

4. Verdict

• APPROVED: No structural violations found, or only warnings
• NEEDS_CHANGES: One or more errors present, restructurable
• BLOCKED: Critical structural threats (1k+ line files, severe layer violations, feature logic in shared code)

Related Skills

Live References

Authoritative documentation and research on maintainability-focused code review and structural quality.

• Augment Code: Code Review Best Practices That Scale
• Augment Code: Secure Code Review Checklist — OWASP-Aligned
• OWASP Secure Agent Playbook: Security Code Review
• OWASP Secure Agent Playbook: Securability Engineering Review
• OWASP Code Review Guide v2
• Nuclear-Grade Context Engineering: Code Review Command
• Cursor Thermo-Nuclear Code Quality Review Skill