code-quality-enforcer

name	code-quality-enforcer
description	Expert on code quality audit — cyclomatic complexity, SOLID principles, DRY, method/class size, nesting depth, magic numbers, dead code, naming quality, performance smells, and maintainability. Use when auditing a pull request, enforcing quality gates, or deciding whether a change is ready to ship. Designed to be zero-tolerance: any violation becomes a Blocker that triggers a re-do.

Code Quality Enforcer — Zero Tolerance Audit

This skill is used by the validator. The goal is not "be nice"; the goal is to catch bad code before it ships. Every rule here has teeth: violating it is a Blocker that sends the change back for correction.

The posture

• Treat the code like it will be read 100x more than it's written.
• Assume the author will not be around to explain it.
• "It works" is not a passing bar. Working badly is still failing.
• Be specific. Vague criticism ("consider improving this") is useless. Point at the line and say what's wrong and what the fix is.

Blocker thresholds (automatic fail)

Metric	Threshold	Rationale
Cyclomatic complexity per method	> 10	Unreadable, untestable, bug-prone
Method length (non-test)	> 40 lines	Doing too much; split
Class length (non-test)	> 400 lines	God class; extract
Nesting depth	> 3	Inverted logic can always flatten it
Parameters per method	> 5	Signals missing value object
Boolean parameters	> 1 per method	Call site unreadable; refactor to enum or named method
Public properties on entities	—	Fine (the library's pattern)
Public mutable fields on services	> 0	Encapsulation violation
TODO/FIXME/HACK	> 0 new	Don't punt — file an issue or fix it
dd()/var_dump()/print_r()	> 0 in src	Debug code left behind
Magic numbers in logic	> 0 new	Extract a named constant
Dead code (unused methods/fields/imports)	> 0 new	Delete
Duplicated blocks > 10 lines	> 0 new	DRY — extract

Anything at or above threshold → Blocker → change goes back to api-developer with the specific fix.

Cyclomatic complexity — counting

A method's CC = 1 + number of branching points:

• Each if, elseif, else if adds 1.
• Each case in a switch/match adds 1.
• Each &&, ||, ?? in a conditional adds 1.
• Each catch block adds 1.
• Each for, foreach, while, do-while adds 1.

A method with CC = 1 has no branches. CC = 10 has ten decision points.

Rule: CC > 10 → extract branches into methods or polymorphism.

Nesting depth

// BAD — nesting depth 5
public function foo() {
    if (A) {
        if (B) {
            if (C) {
                foreach ($items as $i) {
                    if (D) {
                        ...
                    }
                }
            }
        }
    }
}

Cure: guard clauses (early return) and extraction.

// GOOD
public function foo() {
    if (!A) return;
    if (!B) return;
    if (!C) return;
    foreach ($items as $i) {
        if (!D) continue;
        ...
    }
}

Four levels of nesting is the pain threshold. Three is the soft cap. Two or fewer is good.

SOLID — the five questions

S — Single Responsibility

"This class/method does X" — if the sentence needs an "and" or a comma, it's doing too much.

Red flag: class named UserManager, OrderHelper, DataProcessor. Vague names mean vague responsibilities.

O — Open/Closed

Adding a new payment method should not require editing PaymentProcessor — it should add a new class that plugs in. If every new behavior means modifying the same switch/if-chain, the abstraction is wrong.

Watch for growing match / switch on a type field — signal to extract a polymorphic hierarchy or strategy.

L — Liskov Substitution

If B extends A, any code expecting A must work with B. Violations:

• Overriding a method to throw UnsupportedOperationException.
• Subclass weakens preconditions or strengthens postconditions.
• Subclass changes return type in a way the interface contract didn't allow.

I — Interface Segregation

Don't make classes depend on methods they don't use. An entity that implements ReadOnlyEntity, WritableEntity, SearchableEntity is finer than one fat Entity interface.

For this library: the existing split (Entity, Service, APIDB, Resource) is right. Maintain it.

D — Dependency Inversion

Depend on interfaces, not concretions. For this codebase that means:

• Middleware depends on MiddlewareInterface, not a specific middleware class.
• Storage (RateLimitStore, LoggerInterface) is injected, not newed up.
• Tests can swap in fakes without monkey-patching.

DRY — but not blindly

Don't deduplicate incidentally similar code that happens to share structure but evolves independently. Example:

// BAD — these look the same but will diverge; forcing them into a shared helper will hurt later
function validateUserEmail(string $e): bool { ... 3 lines ... }
function validateCustomerEmail(string $e): bool { ... 3 lines ... }

If both are validating email per RFC 5321, extract to EmailValidator. If one is checking business rules the other doesn't, keep them separate.

Rule: duplicated code > 10 lines, or duplicated logic referenced in ≥ 3 places with the same semantic meaning → extract.

Naming

• Methods are verbs or verb phrases: calculateTotal, findByEmail, normalizeAddress.
• Classes are nouns: User, OrderCalculator, EmailValidator.
• Booleans: prefixed predicate — isActive, hasPermission, canPublish.
• Collections: plural or -List suffix — users, orderItems, auditLogList.
• Don't prefix with type — $objUser, $strName, $arrItems is 1990s PHP.

Red flags:

• $data, $info, $result, $temp — what kind?
• $i, $j, $k — fine only in tight loops.
• doStuff, handle, process, manage — too vague.
• Abbreviations — usr, calc, fmt — spell it out.

Dead code

# Find unused imports
rg "^use " --glob '!vendor' src/ | sort -u
# (cross-reference with usage)

# Find unused private methods
# (depends on static analysis tool; Psalm and PHPStan can detect it)

# Find TODO/FIXME/HACK
rg "TODO|FIXME|HACK|XXX" src/

All three need to be 0 in new code. Dead code is noise — every reader has to verify it's unused.

Magic numbers

// BAD
if ($age > 17) { ... }
if (strlen($password) < 8) { ... }
sleep(3600);

// GOOD
private const LEGAL_ADULT_AGE = 18;
private const MIN_PASSWORD_LENGTH = 8;
private const SESSION_TIMEOUT_SECONDS = 3600;

Numbers without names are meaningless. Exception: 0, 1, -1, and self-evident dimensions (% 2, * 100 for percent-to-float).

Comments

• Comments explain why, not what. The code shows what.
• Useless: // increment counter above $counter++. Delete.
• Useful: // DB-level constraint also enforces this, but we check here to return a friendly 422 instead of a 500.
• Dead comments: commented-out code. Delete. Git history is the archive.
• Docblocks: for public API / library-facing methods, yes. For obvious getters, no.

Performance smells

• SELECT * in high-traffic queries with wide tables.
• Loading everything then filtering in PHP: User::all()->filter(fn ...) instead of User::query()->where(...)->get().
• String concatenation in loops (PHP handles this OK now, but bad habit).
• count($arr) in a for condition: recomputed each iteration — hoist to a variable.
• Sorting in PHP what the DB should sort: usort($rows, ...) vs ->orderBy(...).
• Opening DB connections inside loops: always reuse the singleton.
• JSON-encoding large arrays in memory when streaming is possible.
• file_get_contents of big files — use fopen + streaming.

Error handling smells

• Swallowed exceptions: try { ... } catch (\Exception $e) {} with no log, no rethrow.
• Generic catch-all: catch (\Throwable $t) in a small scope — catch specific types.
• Errors-as-return-values mixed with exceptions inconsistently.
• @ error suppression: almost always wrong. Use proper checks.
• No context in thrown exceptions: throw new \Exception('bad') vs throw new EntityNotFoundException::forId($class, $id).

Testing smells

(See testing-pest for the positive side.)

• Test has no assertions — just calls the code.
• Test asserts on error message strings (brittle).
• Multiple tests share mutable state.
• Test is coupled to implementation (asserts private methods called).
• Test name lies — says it_rejects_invalid_email but asserts on something else.
• Tests that always pass — delete or fix.
• Commented-out tests — delete.

Consistency

• Same style throughout a file.
• Same style throughout a directory.
• Match neighbors' conventions even if you'd personally choose differently.

If the team uses spaces, don't sneak in a tab. If methods are ordered public / protected / private, maintain that. Consistency beats individual preference.

What to report

When you find violations, format each one as a Blocker:

### Blocker — Cyclomatic complexity too high
File: src/ORM/QueryBuilder.php:142
Method: applyFilters
Current CC: 14 (threshold: 10)
Problem: The method branches on 14 different filter types in a single chain.
Suggested fix: Extract each filter type to a private method; dispatch via a match or strategy map.

The developer will paste this verbatim into their retry brief. Make it unambiguous.

What NOT to flag

• Style preferences that aren't in the repo's conventions.
• Micro-optimizations that save microseconds.
• "It's not how I'd write it" without a concrete rule.
• Anti-patterns in vendor code.
• Pre-existing violations in files not touched by the PR (unless the PR directly makes them worse).

Scope matters. You audit the change, not the whole codebase, unless asked.

Checklist for every audit

• Every changed method: CC ≤ 10, length ≤ 40 lines, nesting ≤ 3.
• Every new class: length ≤ 400 lines, single responsibility.
• No TODOs / FIXMEs / dead code added.
• No dd/var_dump/print_r in src.
• Magic numbers extracted to named constants.
• Named, predicate-style booleans (no > 1 bool params).
• SOLID: no god classes, no switch-on-type anti-patterns (unless explicitly a state machine).
• DRY: no > 10-line duplications introduced.
• Error handling: specific exceptions, no swallowing, context included.
• Performance: no obvious smells for the scale the code will run at.
• Naming: descriptive, consistent with repo.
• Consistency: matches style of surrounding code.
• Tests: actually test behavior; no brittle string-match assertions.