# Code Review Standards

> Enforces a principal-engineer self-review checklist on every code block before output, covering correctness, performance, security, naming, and testability.

- Skill: `paramchordiya/code-review-standards` (Agent Skill)
- Install (CLI): `npx skillmds@latest add paramchordiya/code-review-standards`
- Raw SKILL.md: https://api.skillmd.com/api/skills/paramchordiya/code-review-standards/raw
- Safety review: PASS (external: skill-scanner WARNING, skillspector CAUTION)
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools, Security, Code Review, Secure Coding
- Tags: Best Practices, Code Quality, Code Review, Naming Conventions, Security Checklist, Self Review
- Author: ParamChordiya (https://skillmd.com/u/paramchordiya)
- Updated: 2026-08-22
- Page: https://skillmd.com/skills/paramchordiya/code-review-standards

---


## Intent

This skill makes every code block produced in this session pass an internal principal-engineer code review before it is shown to the user. It is a meta-skill: it does not govern one domain but governs the quality of all output across every domain. The self-review checklist below must be applied mentally before outputting any code block. Code that would fail this checklist must be corrected before output — not flagged as "a potential improvement" but fixed. This skill also defines the naming, commenting, and security standards that apply universally to all code in this session.

This skill is never overridden by domain-specific skills. It applies universally.

---

## 1. Self-Review Checklist

**Apply this checklist internally to every code block before outputting it.** If any item fails, fix it first.

### Correctness
- [ ] Does this function do exactly one thing? If it does two things, split it.
- [ ] Are all edge cases handled: empty input, null/None, zero, negative numbers, empty collections, single-element collections, maximum input size?
- [ ] Is the logic correct for the boundary conditions? (Off-by-one errors, inclusive/exclusive ranges, ≤ vs <?)
- [ ] If there is concurrency, are race conditions possible?
- [ ] If there is recursion, is the base case correct and is stack overflow possible?

### Complexity and Performance
- [ ] Is the time complexity acceptable for the expected input size? (Document it if not obvious)
- [ ] Is there a hidden O(n²) or worse operation? (Nested loops, `in` on a list, repeated sort)
- [ ] Is there an N+1 query pattern? (Database query or API call inside a loop)
- [ ] Are large objects unnecessarily held in memory when streaming/chunking would suffice?
- [ ] For ML code: are there Python loops over arrays that should be vectorized?

### Robustness and Error Handling
- [ ] Is the error handling correct and informative? (Specific exceptions, context in the message)
- [ ] Are all external inputs validated before use?
- [ ] Are all system boundaries (API handlers, CLI entry points) catching exceptions properly?
- [ ] Is there any silent failure? (Empty except, swallowed exception, None returned on error without documentation)

### Readability and Maintainability
- [ ] Would a new team member understand this code in 60 seconds without asking questions?
- [ ] Are all magic numbers replaced with named constants?
- [ ] Does the naming convey intent? (No `data`, `result`, `temp`, `x`, `thing` in non-trivial code)
- [ ] Are comments explaining WHY, not WHAT? (The code explains what)
- [ ] Is the function/method length reasonable? (40 lines max for Python; flag anything longer)

### Security
- [ ] Are there any SQL injection risks? (String interpolation in queries → must use parameterized queries)
- [ ] Are there any path traversal risks? (User input in file paths → must sanitize)
- [ ] Are there any XSS risks? (User input rendered as HTML → must escape)
- [ ] Are there any secrets in the code output? (API keys, passwords, tokens → must not appear)
- [ ] Are any user inputs deserialized without validation? (Pickle deserialization → refuse; JSON → validate schema)
- [ ] Are external commands constructed from user input? (Shell injection → must use argument arrays, not string interpolation)

### Testability
- [ ] Is the code testable? (No hidden global state, dependencies are injectable)
- [ ] Does the function have a single, testable return path?
- [ ] Are all dependencies mockable via interface or injection?

---

## 2. Naming Conventions — Universal Rules

### Functions and Methods
- Always use verb phrases: `calculate_revenue`, `fetch_user_by_id`, `validate_payment`, `parse_config`
- Avoid: `process`, `handle`, `do_thing`, `run` — these are not descriptive enough
- Async functions: same rules; do not prefix with `async_` — the `async def` keyword is sufficient

### Boolean Variables and Functions
- Always use `is_`, `has_`, `can_`, `should_`, `was_`, `needs_` prefixes:
  - `is_active`, `has_premium_subscription`, `can_withdraw`, `should_retry`
  - Predicate functions: `is_valid_email(email)`, `has_sufficient_balance(account)`
- Never: `flag`, `check`, `status` as boolean names

### Classes
- Always nouns or noun phrases, PascalCase: `UserRepository`, `ChurnPredictor`, `TransactionService`
- Avoid: `Manager`, `Helper`, `Util`, `Handler` as class name suffixes — these indicate unclear responsibility

### Constants
- SCREAMING_SNAKE_CASE: `MAX_RETRY_ATTEMPTS`, `DEFAULT_BATCH_SIZE`, `CHURN_PROBABILITY_THRESHOLD`
- Never: magic numbers inline in code

### Variables — Forbidden Names in Non-Trivial Code
The following are forbidden as variable names outside of well-understood mathematical or loop contexts:
`data`, `result`, `temp`, `tmp`, `x`, `y`, `z`, `thing`, `obj`, `item` (unless in a comprehension over a clearly named collection), `val`, `value` (use the semantic name instead), `info`, `stuff`

### Acceptable Single-Letter Variables
Single-letter variables are only acceptable in:
- Loop indices over integer ranges: `i`, `j`, `k`
- Mathematical formulas where the letter has a standard meaning: `n` for count, `m` for rows, `k` for clusters
- Lambda expressions with trivially obvious meaning: `sorted(items, key=lambda x: x.timestamp)`

---

## 3. Comment Philosophy — Universal Rules

**Comments explain WHY, not WHAT.** The code says what; the comment says why.

```python
# WRONG — explains what (redundant with the code)
# Increment counter by 1
counter += 1

# WRONG — explains what
# Check if user is active
if user.is_active:
    ...

# CORRECT — explains why
# Use exponential backoff capped at 60s: Stripe's API has a 429 rate limit
# that resets on a per-minute window. Backoff prevents thundering herd on retry.
wait_seconds = min(60, 2 ** attempt)
```

**Every non-obvious algorithmic step must have a comment.** "Non-obvious" means: if you removed the comment, a reasonable senior engineer would need more than 10 seconds to understand why that specific approach was taken.

**Every `TODO` must include three things:**
```python
# TODO(param): Switch from PSI to KS test for continuous features.
# Deferred because: PSI is computed for all feature types currently;
# KS implementation requires distribution fitting step.
# Track: https://github.com/myorg/myrepo/issues/47
```

**Commented-out code must be deleted.** Version control exists for archaeology. If code is left commented out, it must have an expiry comment or it will be deleted on the next review:
```python
# REMOVED 2024-03-15: replaced by vectorized numpy approach (see commit abc1234)
# Keeping for reference until performance regression testing is complete (deadline: 2024-04-01).
# old_approach = [compute(x) for x in data]
```

---

## 4. Security Review — Applied Automatically

**SQL injection: always use parameterized queries. Never interpolate user input into SQL strings.**

```python
# FORBIDDEN — SQL injection vulnerability
query = f"SELECT * FROM users WHERE email = '{email}'"
cursor.execute(query)

# CORRECT — parameterized query
cursor.execute("SELECT * FROM users WHERE email = %s", (email,))

# CORRECT with SQLAlchemy ORM
user = session.query(User).filter(User.email == email).first()
```

**Path traversal: always validate and sanitize file paths derived from user input.**

```python
# FORBIDDEN — path traversal vulnerability
file_path = Path(base_dir) / user_supplied_filename
with open(file_path) as f: ...

# CORRECT — resolve and verify the path is under the allowed base directory
def safe_open(base_dir: Path, user_filename: str) -> Path:
    safe_path = (base_dir / user_filename).resolve()
    if not safe_path.is_relative_to(base_dir.resolve()):
        raise PermissionError(f"Path traversal attempt detected: {user_filename}")
    return safe_path
```

**Shell injection: never construct shell commands from user input using string interpolation.**

```python
# FORBIDDEN — shell injection vulnerability
os.system(f"convert {user_input_filename} output.pdf")
subprocess.run(f"git clone {repo_url}", shell=True)

# CORRECT — use argument arrays, shell=False
subprocess.run(["convert", user_input_filename, "output.pdf"], check=True)
subprocess.run(["git", "clone", repo_url], check=True)
```

**Deserialization: never unpickle data from untrusted sources. Use JSON with schema validation instead.**

```python
# FORBIDDEN — arbitrary code execution via malicious pickle
model = pickle.loads(user_uploaded_bytes)

# CORRECT — validate before deserializing; for models, use MLflow's safe loading
model = mlflow.pyfunc.load_model(model_uri)  # MLflow validates the artifact
```

**Secrets detection: before outputting any code, scan for patterns that look like credentials:**
- Strings matching patterns like `sk-...`, `ghp_...`, `AKIA...`, `-----BEGIN PRIVATE KEY-----`
- Hardcoded passwords, API keys, connection strings with credentials
- If found: replace with a placeholder and add a comment directing the user to use a secrets manager

---

## 5. Performance Review — Applied to Data-Scale Code

**For any loop over a dataset, estimate the complexity and flag O(n²) or worse:**

When generating code that processes collections:
1. Estimate n (explicitly — "what is the expected input size?")
2. State the complexity
3. If O(n²) or worse, offer a better approach unless O(n²) is justified

**For any database query generated, ensure it uses indexes. Flag full table scans:**

```sql
-- FLAGGED: No index on user_id? This is a full table scan on orders for every user lookup.
SELECT * FROM orders WHERE user_id = 123;

-- REQUIRES: CREATE INDEX idx_orders_user_id ON orders(user_id);
-- Or: ensure user_id is the partition key in distributed storage.
```

**For any API call in a loop, flag as an N+1 problem and suggest batching:**

```python
# FLAGGED: N+1 pattern — makes one API call per user. For 10,000 users: 10,000 API calls.
for user in users:
    profile = api.get_user_profile(user.id)  # N+1

# CORRECT: batch API call
user_ids = [u.id for u in users]
profiles = api.get_user_profiles_batch(user_ids)  # 1 call, or ceil(n/batch_size) calls
```

**For any large object held in memory, suggest streaming or chunking if size is unbounded:**

```python
# FLAGGED: loads entire file into memory. For files > available RAM, this will OOM.
data = json.load(open("huge_dataset.json"))

# CORRECT: stream with ijson or process in chunks
import ijson
with open("huge_dataset.json", "rb") as f:
    for record in ijson.items(f, "item"):
        process(record)
```