---
description: Review code against best practices and guidelines with specific checks
argument-hint: [path] [--duplication] [--prefer-classes] [--all] [--strict] [--compare]
allowed-tools: Task, Read, Write, Bash, Grep, Glob
---

# Best Practices Review: $ARGUMENTS

**Note**: This command reviews code against language-specific best practices from `~/.claude/guides/`. It complements `/architect:clean` (architecture violations) and `/architect:kiss` (complexity reduction) by focusing on coding standards and design patterns. Use together for comprehensive code quality analysis.

## Phase 0: Parse Arguments

Extract from `$ARGUMENTS`:

- **Path** (first positional argument): Optional scope for analysis
  - File path (e.g., `src/utils/auth.ts`) → Analyze specific file
  - Directory path (e.g., `src/`) → Analyze all files in directory
  - Pattern (e.g., `src/**/*.ts`) → Analyze matching files
  - Empty or `--diff` → Analyze git-changed files only
- **Checks** (flags): Specific checks to run
  - `--duplication`: Check for code duplication opportunities
  - `--prefer-classes`: Check for functions that should be class methods
  - `--all`: Run all available checks (default if no specific checks)
- **Options**:
  - `--strict`: Fail if violations exceed threshold
  - `--compare`: Compare with previous `.architect.review.local.md` report

**Smart scope resolution:**

```bash
# Parse arguments
SCOPE=""
CHECKS=()
OPTIONS=()

for arg in $ARGUMENTS; do
  case "$arg" in
    --duplication|--prefer-classes|--all)
      CHECKS+=("$arg")
      ;;
    --strict|--compare|--diff)
      OPTIONS+=("$arg")
      ;;
    *)
      # First non-flag argument is the scope
      if [[ -z "$SCOPE" ]]; then
        SCOPE="$arg"
      fi
      ;;
  esac
done

# Default to --all if no specific checks
if [[ ${#CHECKS[@]} -eq 0 ]]; then
  CHECKS=("--all")
fi

# Determine mode
if [[ -z "$SCOPE" ]] || [[ " ${OPTIONS[*]} " =~ " --diff " ]]; then
  MODE="diff"
  SCOPE=$(git diff --name-only origin/main...HEAD 2>/dev/null | grep -E '\.(ts|js|tsx|jsx|py|go|rb)$')
  echo "Analyzing git-changed files only"
elif [[ -f "$SCOPE" ]]; then
  MODE="file"
  echo "Analyzing single file: $SCOPE"
elif [[ -d "$SCOPE" ]]; then
  MODE="directory"
  echo "Analyzing directory: $SCOPE"
elif [[ "$SCOPE" == *"*"* ]]; then
  MODE="pattern"
  echo "Analyzing pattern: $SCOPE"
fi
```

## Phase 1: Detect Language and Load Guidelines

### 1.1 Detect Primary Language

Analyze files in scope to determine primary language:

```typescript
// Language detection logic
interface LanguageStats {
  language: string;
  fileCount: number;
  extension: string;
}

const languageMap: Record<string, string> = {
  '.ts': 'typescript',
  '.tsx': 'typescript',
  '.js': 'javascript',
  '.jsx': 'javascript',
  '.py': 'python',
  '.go': 'go',
  '.rb': 'ruby',
  '.rs': 'rust',
  '.java': 'java',
  '.cs': 'csharp',
  '.php': 'php',
};

// Count files by extension in scope
const stats = countFilesByExtension(scopeFiles);
const primaryLanguage = stats[0].language; // Most common
```

### 1.2 Load Language Guidelines

Fetch guidelines from `~/.claude/guides/`:

```bash
# Check for language-specific guidelines
GUIDES_DIR="$HOME/.claude/guides"
LANGUAGE="typescript"  # Detected from Phase 1.1

# Look for guidelines in order of specificity
GUIDELINES_FILES=(
  "$GUIDES_DIR/agents.${LANGUAGE}.best-practices.md"
  "$GUIDES_DIR/agents.${LANGUAGE}.md"
  "$GUIDES_DIR/agents.clean.arch.${LANGUAGE}.md"
  "$GUIDES_DIR/agents.tdd.${LANGUAGE}.md"
)

LOADED_GUIDELINES=()
for file in "${GUIDELINES_FILES[@]}"; do
  if [[ -f "$file" ]]; then
    LOADED_GUIDELINES+=("$file")
    echo "Loaded guidelines: $file"
  fi
done

if [[ ${#LOADED_GUIDELINES[@]} -eq 0 ]]; then
  echo "Warning: No language-specific guidelines found for $LANGUAGE"
  echo "Using generic best practices"
fi
```

## Phase 2: Run Specific Checks

### 2.1 Duplication Check (`--duplication`)

**Purpose**: Identify code duplication and suggest base class extraction or shared utilities.

**What it checks:**

1. **Similar code blocks** - Functions/methods with >70% similarity
2. **Copy-paste patterns** - Identical code in multiple files
3. **Extractable base classes** - Classes with duplicate methods
4. **Shared utility candidates** - Repeated logic across modules

**Analysis approach:**

```typescript
// Launch duplication analysis agent
await Task({
  subagent_type: 'code-reviewer',
  description: 'Check code duplication',
  prompt: `Analyze for code duplication in scope: ${scope}

Guidelines loaded: ${loadedGuidelines.join(', ')}

Check for:
1. Functions/methods with similar logic (>70% overlap)
2. Copy-paste code blocks (identical or near-identical)
3. Classes that should share a base class
4. Repeated patterns that should be utility functions
5. Similar error handling that could be centralized

For each duplication found, suggest:
- Whether to extract base class (OOP approach)
- Whether to create shared utility (functional approach)
- Whether to use composition over inheritance
- How to maintain clean architecture principles

Output format:
- File locations with line numbers
- Similarity percentage
- Recommended refactoring approach
- Code example of suggested extraction`,
});
```

**Duplication severity levels:**

| Similarity | Severity | Recommendation                          |
| ---------- | -------- | --------------------------------------- |
| >90%       | Critical | Immediate extraction required           |
| 70-90%     | High     | Extract to shared utility/base class    |
| 50-70%     | Medium   | Consider abstraction                    |
| <50%       | Low      | May be intentional, document if keeping |

### 2.2 Prefer Classes Check (`--prefer-classes`)

**Purpose**: Identify exported functions that should be organized into classes with static methods for cleaner barrel exports.

**What it checks:**

1. **Function groups with common prefix** - Functions like `createUser`, `updateUser`, `deleteUser` → `UserService` class
2. **Stateless utility functions** - Related functions that could be static class methods
3. **Factory functions** - Functions returning objects that could be class constructors
4. **Namespace-like exports** - Multiple exports that logically belong together

**Analysis approach:**

```typescript
// Launch class extraction analysis agent
await Task({
  subagent_type: 'code-reviewer',
  description: 'Check function-to-class opportunities',
  prompt: `Analyze for functions that should be class methods in scope: ${scope}

Guidelines loaded: ${loadedGuidelines.join(', ')}

Check for:
1. Functions with common prefix (e.g., userCreate, userUpdate, userDelete)
2. Groups of 3+ related exported functions
3. Functions that share common dependencies
4. Functions that would benefit from static method organization
5. Files with many standalone exports that complicate barrel files

For each opportunity found:
- Current: Multiple exported functions
- Suggested: Class with static methods OR instance class with DI
- Rationale: Why class organization is cleaner
- Barrel impact: How it simplifies index.ts exports

Example transformation:
// Before (function exports)
export function createUser(data: UserData): User { ... }
export function updateUser(id: string, data: Partial<UserData>): User { ... }
export function deleteUser(id: string): void { ... }
export function getUserById(id: string): User | null { ... }

// After (class with static methods)
export class UserService {
  static create(data: UserData): User { ... }
  static update(id: string, data: Partial<UserData>): User { ... }
  static delete(id: string): void { ... }
  static getById(id: string): User | null { ... }
}

// OR (class with DI for dependencies)
export class UserService {
  constructor(private readonly db: IDatabase) {}
  create(data: UserData): User { ... }
  update(id: string, data: Partial<UserData>): User { ... }
  // ...
}
export const defaultUserService = new UserService(defaultDatabase);

Benefits:
- Cleaner barrel: export { UserService } vs export { createUser, updateUser, ... }
- Logical grouping: Related functionality in one class
- Easier mocking: Mock entire class vs individual functions
- Better IDE support: Autocomplete shows all related methods`,
});
```

**Prefer-classes severity levels:**

| Pattern                    | Severity | Recommendation            |
| -------------------------- | -------- | ------------------------- |
| 5+ related functions       | High     | Extract to class          |
| 3-4 related functions      | Medium   | Consider class extraction |
| Functions with shared deps | High     | Use class with DI         |
| Common prefix pattern      | Medium   | Group into service class  |

## Phase 3: Generate Review Report

**Create `.tmp/agent/.architect.review.local.md` with findings:**

````markdown
# Best Practices Review Report

**Generated:** [timestamp ISO]
**Scope:** [analyzed scope]
**Mode:** [file | directory | pattern | diff]
**Language:** [detected language]
**Checks run:** [list of checks]
**Guidelines loaded:** [list of guideline files]

---

## Executive Summary

**Total Issues:** [count]

- Critical: [count] (Must fix before merge)
- High: [count] (Should fix soon)
- Medium: [count] (Improve when possible)
- Low: [count] (Nice-to-have)

**Checks Summary:**

| Check          | Issues  | Critical | High    | Medium  | Low     |
| -------------- | ------- | -------- | ------- | ------- | ------- |
| Duplication    | [count] | [count]  | [count] | [count] | [count] |
| Prefer Classes | [count] | [count]  | [count] | [count] | [count] |

**Overall Assessment:** [summary]

---

## Duplication Analysis

### Critical Duplication (>90% Similarity)

#### 1. [Description] - [File1] ↔ [File2]

**Similarity:** [percentage]%

**Location 1:** `[file1:line-start:line-end]`

```[language]
[code snippet 1]
```

**Location 2:** `[file2:line-start:line-end]`

```[language]
[code snippet 2]
```

**Recommendation:** Extract to base class / shared utility

**Suggested extraction:**

```[language]
// New file: src/shared/[name].ts
[extracted code with proper abstraction]
```

**Refactored usage:**

```[language]
// In file1
import { ExtractedClass } from '../shared/[name]';
// ... usage
```

---

[Repeat for each duplication issue]

### High Priority Duplication (70-90%)

[Same structure as critical]

### Medium Priority Duplication (50-70%)

[Same structure as critical]

---

## Prefer Classes Analysis

### High Priority - Multiple Related Functions

#### 1. [File] - [Count] related functions found

**Current exports:**

```[language]
// [file]
export function [prefix]Create(...) { ... }
export function [prefix]Update(...) { ... }
export function [prefix]Delete(...) { ... }
export function [prefix]GetById(...) { ... }
export function [prefix]List(...) { ... }
```

**Problem:**

- [count] functions with common prefix `[prefix]`
- Barrel file (`index.ts`) requires [count] individual exports
- Related functionality scattered across file
- Harder to mock in tests (mock each function)

**Recommended refactoring:**

**Option A: Static methods (stateless operations)**

```[language]
// [file] - refactored
export class [PrefixService] {
  static create(...): [Type] { ... }
  static update(...): [Type] { ... }
  static delete(...): void { ... }
  static getById(...): [Type] | null { ... }
  static list(...): [Type][] { ... }
}
```

**Option B: Instance class with DI (has dependencies)**

```[language]
// [file] - refactored
export class [PrefixService] {
  constructor(
    private readonly [dep1]: I[Dep1],
    private readonly [dep2]: I[Dep2]
  ) {}

  create(...): [Type] { ... }
  update(...): [Type] { ... }
  // ...
}

// Exported singleton with default dependencies
export const default[PrefixService] = new [PrefixService](
  default[Dep1],
  default[Dep2]
);
```

**Barrel simplification:**

```[language]
// Before: index.ts
export {
  [prefix]Create,
  [prefix]Update,
  [prefix]Delete,
  [prefix]GetById,
  [prefix]List,
} from './[file]';

// After: index.ts
export { [PrefixService] } from './[file]';
// OR
export { [PrefixService], default[PrefixService] } from './[file]';
```

**Impact:**

- Exports reduced: [before] → [after]
- Test mocking simplified
- Related code grouped logically

---

[Repeat for each prefer-classes issue]

---

## Guidelines Applied

### Loaded Guidelines

| Guideline File | Principles Applied   |
| -------------- | -------------------- |
| [file1]        | [list of principles] |
| [file2]        | [list of principles] |

### Key Principles Referenced

1. **[Principle 1]**: [How it was applied]
2. **[Principle 2]**: [How it was applied]
3. **[Principle 3]**: [How it was applied]

---

## Recommendations

### Immediate Actions (Critical Issues)

1. **[Action 1]**
   - Files affected: [list]
   - Effort: [low/medium/high]
   - Impact: [description]

### Short-term Improvements (High Priority)

[Same structure]

### Long-term Enhancements (Medium/Low Priority)

[Same structure]

---

## Next Steps

### Option 1: Fix Issues Individually

```bash
# Open report and address issues one by one
# For each issue, follow the suggested refactoring
```

### Option 2: Convert to Tasks

```bash
/task:add "Extract duplicate code in [module]"
/task:add "Refactor [functions] to [Service] class"
```

### Option 3: Run with --strict in CI

```bash
# Add to pre-commit or CI
/architect:review --strict
# Fails if critical or high issues found
```

### Option 4: Compare Over Time

```bash
# Run again after fixes
/architect:review --compare
# Shows improvement metrics
```

---

## Comparison with Previous Report

[Only if --compare used]

### Changes Since Last Analysis ([date])

**Issues resolved:** [count]
**New issues:** [count]
**Net change:** [±count]

### Metric Trends

| Metric        | Previous | Current | Trend |
| ------------- | -------- | ------- | ----- |
| Total issues  | [count]  | [count] | [↑↓→] |
| Critical      | [count]  | [count] | [↑↓→] |
| Duplication % | [pct]    | [pct]   | [↑↓→] |

---

## Configuration

**Checks run:**

- `--duplication`: [enabled/disabled]
- `--prefer-classes`: [enabled/disabled]

**Thresholds:**

- Duplication similarity: 50%+ (warning), 70%+ (high), 90%+ (critical)
- Related functions: 3+ (medium), 5+ (high)

**Excluded paths:**

- `node_modules/**`
- `dist/**`
- `**/*.test.ts`
- `**/*.d.ts`

---

**Report generated by:** Best Practices Review
**Report location:** `.tmp/agent/.architect.review.local.md`
**Execution time:** [duration]
````

## Phase 4: Present Results to User

### 4.1 Ensure Output Directory

```bash
# Create output directory
mkdir -p .tmp/agent
```

### 4.2 Write Report to File

```bash
# Write report to .tmp/agent/.architect.review.local.md
cat > .tmp/agent/.architect.review.local.md <<EOF
[Generated report from Phase 3]
EOF
```

### 4.3 Console Summary

Print concise summary to console:

```text
Best Practices Review Complete

Scope: [analyzed scope]
Language: [detected language]
Checks: [list of checks run]

Issues found: [total count]
  Critical: [count]
  High: [count]
  Medium: [count]
  Low: [count]

Duplication:
  - [count] code blocks with >70% similarity
  - [count] extraction opportunities

Prefer Classes:
  - [count] function groups could be classes
  - [count] barrel exports could be simplified

Guidelines applied: [count] from ~/.claude/guides/

Report: .tmp/agent/.architect.review.local.md

Next steps:
  1. Review findings in report
  2. Address critical issues first
  3. Run /architect:review --compare after fixes
```

### 4.4 Strict Mode Check

```bash
if [[ " ${OPTIONS[*]} " =~ " --strict " ]]; then
  if [[ $CRITICAL_COUNT -gt 0 ]] || [[ $HIGH_COUNT -gt 0 ]]; then
    echo "STRICT MODE: Found $CRITICAL_COUNT critical and $HIGH_COUNT high issues"
    exit 1
  fi
fi
```

## Error Handling

**No files in scope:**

- Warning: "No files found matching scope: [scope]"
- Exit with status 0

**No guidelines found:**

- Warning: "No language-specific guidelines found for [language]"
- Continue with generic best practices
- Exit with status 0

**Invalid check flag:**

- Error: "Unknown check flag: [flag]"
- Show available flags: `--duplication`, `--prefer-classes`, `--all`
- Exit with status 1

**Analysis failure:**

- Error: "Analysis failed: [reason]"
- Exit with status 2

## Exit Codes

- 0: Analysis completed successfully
- 1: Invalid arguments or configuration
- 2: Analysis execution failure
- 3: Strict mode violations found

## Quick Reference

**Common workflows:**

```bash
# Full review with all checks (default)
/architect:review

# Review specific directory
/architect:review src/utils/

# Run specific checks only
/architect:review --duplication
/architect:review --prefer-classes
/architect:review src/ --duplication --prefer-classes

# Strict mode for CI/pre-commit
/architect:review --strict

# Compare with previous analysis
/architect:review --compare

# Review git-changed files only
/architect:review --diff
```

**Available checks:**

| Flag               | Description                                     |
| ------------------ | ----------------------------------------------- |
| `--duplication`    | Find code duplication, suggest extractions      |
| `--prefer-classes` | Find function groups, suggest class refactoring |
| `--all`            | Run all checks (default if no specific checks)  |

**Integration with other commands:**

```bash
# Full architecture review workflow
/architect:clean src/          # Fix architecture violations
/architect:kiss src/           # Reduce complexity
/architect:review src/         # Apply best practices

# Convert findings to tasks
/architect:review src/
/task:add "Extract duplicate validation logic"
/task:add "Refactor user functions to UserService class"
```