Code reviewer

BoxLang Code Reviewer

Overview

This skill provides a structured checklist and framework for reviewing BoxLang code. Apply these checks systematically when reviewing PRs, auditing existing code, or self-reviewing before committing.

---

Review Framework

When reviewing BoxLang code, evaluate these categories in order of priority:

1. Security — Vulnerabilities that could be exploited
2. Correctness — Logic errors, edge cases, null safety
3. Performance — Inefficiencies, unnecessary work
4. Maintainability — Readability, naming, structure
5. Style — Conventions, consistency

---

Security Checks

SQL Injection

// RED FLAG — string interpolation in SQL
queryExecute( "SELECT * FROM users WHERE id = #url.id#" )

// REQUIRED FIX — always parameterize
queryExecute(
    "SELECT * FROM users WHERE id = :id",
    { id: { value: url.id, cfsqltype: "cf_sql_integer" } }
)

Review question: Is every SQL value passed via queryParam / :name binding?

XSS (Cross-Site Scripting)

// RED FLAG — raw user input rendered in HTML
<bx:output>#form.comment#</bx:output>

// REQUIRED FIX — encode for context
<bx:output>#encodeForHTML( form.comment )#</bx:output>

Review question: Is every user-supplied value encoded with the appropriate encodeFor*() function before output?

File Upload Validation

• Is the file extension validated against an allowlist?
• Is MIME type validated server-side (not just by browser)?
• Are files stored outside the webroot?
• Is file size limited?

Path Traversal

Review question: Does any file read/write accept a user-supplied path? If yes, is it validated with canonical path comparison?

Secrets in Code

RED FLAG: API keys, passwords, tokens hardcoded in .bx, .bxs, .bxm, or boxlang.json.

REQUIRED FIX: Use ${env.SECRET_NAME} in config, and access via server.system.environment.SECRET_NAME in code.

Remote Function Exposure

Review question: Are remote functions authenticated before executing? Are all arguments validated before use?

---

Correctness Checks

Variable Scoping

// RED FLAG — missing var keyword (bleeds into variables scope)
function process() {
    result = loadData()    // BAD
    return result
}

// CORRECT
function process() {
    var result = loadData()
    return result
}

Null Safety

// RED FLAG — potential null pointer
var name = user.profile.displayName   // throws if profile is null

// CORRECT — null-safe navigation
var name = user?.profile?.displayName ?: "Anonymous"

Exception Handling

// RED FLAG — catch-all silently swallows bugs
try {
    doWork()
} catch ( any e ) {
    logError( e )   // continues execution after unexpected error
}

// CORRECT — re-throw unknown errors
try {
    doWork()
} catch ( "ExpectedError" e ) {
    handleExpected( e )
} catch ( any e ) {
    logError( e )
    rethrow   // let unexpected errors propagate
}

Edge Cases to Check

• What happens when the input is empty / null / zero?
• What happens at collection boundary (first and last element)?
• What happens when a database/API call returns no rows?
• Are all required struct keys guarded with structKeyExists()?

---

Performance Checks

Scope Access in Loops

// SLOWER — scope chain walked every iteration
for ( var i = 1; i <= items.len(); i++ ) {
    process( items[ i ] )
}

// FASTER — len() called once
var count = items.len()
for ( var i = 1; i <= count; i++ ) {
    process( items[ i ] )
}

N+1 Query Pattern

// RED FLAG — query inside a loop (N+1 problem)
for ( var order in orders ) {
    order.customer = queryExecute( "SELECT * FROM customers WHERE id = #order.customerId#" )
}

// CORRECT — fetch all in one query with JOIN or batch lookup
var enriched = queryExecute(
    "SELECT o.*, c.name as customerName
     FROM orders o
     JOIN customers c ON c.id = o.customerId
     WHERE o.status = :status",
    { status: { value: "active", cfsqltype: "cf_sql_varchar" } }
)

Application Scope Cache Writes

// RED FLAG — unguarded write to application scope (race condition)
application.settings = loadSettings()

// CORRECT — use locking
bx:lock name="app-settings-lock" type="exclusive" timeout="5" {
    application.settings = loadSettings()
}

---

Maintainability Checks

Naming

Check	Bad Example	Good Example
Variables clear	var x = getData()	var userProfile = getProfile( id )
Functions descriptive	function do()	function processPayment()
Boolean names readable	var flag = check()	var isEligible = checkEligibility()
Magic numbers named	if ( status == 3 )	if ( status == STATUS_SUSPENDED )

Function Length

Flag functions exceeding ~50 lines — they likely need splitting. Each function should do one thing clearly.

Argument Declarations

// RED FLAG — no argument metadata
function getUser( id, options ) { ... }

// REQUIRED — typed, required annotations
function getUser( required numeric id, struct options = {} ) { ... }

Dead Code

Flag:

• Variables declared but never read
• Functions defined but never called
• Commented-out blocks left in production code
• Conditions that can never be true

---

Style Checks

Semicolons

BoxLang docs state: do not use semicolons except where required.

// BAD — unnecessary semicolons
var name = "BoxLang";
return result;

// CORRECT — no semicolons needed
var name = "BoxLang"
return result

// OK — semicolons required here
property name="userId" type="numeric";

Closures vs Lambdas

// Correct — lambda for pure transforms (no outer scope)
var doubled = numbers.map( ( n ) -> n * 2 )

// Correct — closure for outer scope access or BIF calls
var filtered = numbers.filter( ( n ) => n > minValue )
var upper    = words.map( ( w ) => uCase( w ) )

Struct Literals

// Acceptable but verbose
var user = structNew()
user.name = "Alice"

// Preferred — literal syntax
var user = { name: "Alice", email: "[email protected]" }

---

Review Output Template

When writing review feedback, use this structure:

## Critical (must fix before merge)

- [ ] **[Security/SQL Injection]** Line 42: `url.id` is interpolated directly into SQL.
      Fix: use `:id` binding with `cfsqltype: "cf_sql_integer"`.

- [ ] **[Security/XSS]** Line 87: `form.description` rendered without encoding.
      Fix: `encodeForHTML( form.description )`.

## Major (should fix)

- [ ] **[Correctness/Null Safety]** Line 23: `user.address.city` — `address` may be null.
      Fix: use `user?.address?.city ?: "Unknown"`.

- [ ] **[Performance/N+1]** Lines 55–60: query inside loop will execute N queries.
      Fix: use JOIN or batch lookup.

## Minor (consider fixing)

- [ ] **[Style/Naming]** `var x` on line 31 — rename to clarify purpose.
- [ ] **[Docs]** `processOrder()` lacks `@param` and `@return` documentation.

## Positive Feedback

- Good use of parameterized queries in `getUserById()`.
- Error handling in `chargePayment()` correctly re-throws unknown exceptions.

---

Automated Tools

Run these before manual review:

# Format check (BoxLang code style)
box run-script format:check

# Run test suite
./run --reporter=text

# Check for outdated modules (dependency audit)
box outdated

# Security scan (if using CommandBox security module)
box security:scan