code-quality

What this skill does

# Code Quality

Good code is code the next person can read, change, and trust. Where correctness is established by tests and verification, quality is everything those do not catch: clarity, maintainability, and fitting the code that already exists. Apply this both when implementing (so the diff is right the first time) and when reviewing (so it is judged honestly).

## Ground yourself in the existing code first

Before judging or writing anything, learn how this repo already does it. The codebase's own conventions outrank generic best practices — a "better" pattern that nothing else uses is itself an inconsistency.

- `Glob` for sibling files of the same kind (`**/*Service.ts`, `**/handlers/*.py`) to see the established shape.
- `Grep` for how a function, type, or error is used elsewhere before you assume how it should be used here.
- `Read` a couple of representative files to absorb the error-handling style, naming, and module layout.

If you are deliberately deviating from an existing pattern, that is allowed — but the deviation must be explicit (a comment or PR note saying why), never silent. Silent deviation reads as a mistake to every future reader.

## What to evaluate

### Readability and structure
- Can you understand each function without scrolling back and forth? If not, it is doing too much.
- Deep nesting hides the happy path. Flatten with early returns / guard clauses.
- Long functions mixing levels of abstraction (parsing + business logic + I/O) should be split so each does one thing.
- Magic numbers and bare strings should be named constants when their meaning is not obvious from context.

### Naming
- Names describe intent, not type or implementation (`activeUsers`, not `list2`). A reader should predict what a thing does from its name.
- Match the repo's casing and vocabulary. If the codebase says `fetch`, do not introduce `get`/`load`/`retrieve` for the same idea — inconsistent synonyms make the code seem like it has distinctions it does not.

### Error handling
- Follow the codebase's existing strategy (exceptions vs. result types vs. error returns). Mixing strategies forces every caller to handle two worlds.
- Handle the failure cases that can actually happen here; do not swallow errors silently or catch-and-ignore. An empty `catch` is a future debugging session.
- Error messages should give the reader enough to act (what failed, with what input).

### Abstraction — appropriate, not premature
- Reuse an existing helper instead of re-implementing it. Duplicated logic drifts apart over time.
- But do not invent a framework for one caller. The right abstraction is the one the current requirements justify, not a speculative one.

### Dead and leftover code
- No unused functions, variables, imports, or commented-out blocks. Version control already remembers the old code; leaving it in the file only confuses.
- No debug prints / leftover scaffolding from the implementation session.

### Tests
- New behavior has tests; changed behavior has updated tests. Verification can only confirm what tests exercise — untested behavior cannot be confirmed.
- Tests cover the edge cases the code handles (empty, boundary, error paths), and follow the repo's existing test structure and naming.

## Common code smells

| Smell | Why it hurts | Fix |
|-------|--------------|-----|
| Long method | Hard to read, test, reuse | Extract smaller functions |
| Deep nesting | Obscures the main path | Early returns / guard clauses |
| N+1 query | Silent performance cliff | Eager-load / batch |
| God object | Change ripples everywhere | Split responsibilities |
| Copy-pasted logic | Drifts out of sync | Reuse a shared helper |
| Dead / commented code | Confuses readers | Delete it |

## Verify before you flag

A flag that turns out to be the house style wastes the team's trust. After spotting an issue, `Grep` the codebase to confirm it is actually anomalous. If the "problem" pattern is used consistently everywhere, the existing convention wins — report it only if it is genuinely harmful.

## Report format

For each finding:
- **Severity**: Critical / High / Medium / Low
- **Location**: `file:line`
- **Issue**: what is wrong, in one sentence
- **Suggestion**: the specific change to make

## Examples

Early returns over nesting:

```python
# Hard to follow — the real work is buried three levels deep
def process(data):
    if data:
        if data.is_valid():
            if data.has_permission():
                return do_work(data)

# Each precondition is explicit and the happy path is flat
def process(data):
    if not data:
        return None
    if not data.is_valid():
        raise ValueError("invalid data")
    if not data.has_permission():
        raise PermissionError("access denied")
    return do_work(data)
```

Avoid the N+1 query — it passes tests but degrades silently at scale:

```python
# One query per user
for user in users:
    posts = db.query(Post).filter(Post.user_id == user.id).all()

# One query total
users = db.query(User).options(joinedload(User.posts)).all()
```