legacy-code-reviewer skill

---
name: Legacy Code Reviewer
description: Expert system for identifying deprecated patterns, suggesting refactoring to modern standards (Python 3.12+, ES2024+), checking test coverage, and leveraging AI-powered tools. Proactively applied when users request refactoring, updates, or analysis of legacy codebases.
allowed-tools: Read, Grep, Glob, Web Search
last-updated: 2025-12-15
---

# Legacy Code Review and Refactoring Plan

## Core Philosophy

Legacy code review is not just about finding bugs—it's about understanding the **original intent**, reducing technical debt, and incrementally modernizing while maintaining stability. Apply the principle: *"Make it work, make it right, make it fast"* in that order.

---

## Instructions

### 1. Discovery & Analysis

**Use Read, Grep, and Glob tools to:**
- Identify files referenced by the user
- Search for deprecated APIs, patterns, or libraries (e.g., `collections.MutableMapping` → `collections.abc.MutableMapping`)
- Detect code smells:
  - Unused imports/functions (candidates for deletion)
  - Duplicated code blocks (refactor to DRY)
  - High cyclomatic complexity (McCabe > 10)
  - Missing type hints (Python) or JSDoc (JavaScript)

### 2. Modern Tooling Recommendations

**Python Legacy Code (2024-2025 Best Practices):**

| Tool | Purpose | Usage |
|------|---------|-------|
| **Ruff** | Ultra-fast linter + formatter (replaces Flake8, Black, isort) | `ruff check --fix .` |
| **Vulture** | Dead code detection | `vulture src/` |
| **Refurb** | Suggests modern Python idioms | `refurb check src/` |
| **mypy** | Static type checking | `mypy --strict src/` |
| **Bandit** | Security vulnerability scanner | `bandit -r src/` |
| **Radon** | Complexity metrics | `radon cc -s src/` |
| **pytest** + **coverage** | Test coverage analysis | `pytest --cov=src tests/` |

**JavaScript/TypeScript Legacy Code:**

| Tool | Purpose | Usage |
|------|---------|-------|
| **ESLint** (with `eslint-plugin-deprecation`) | Detect deprecated APIs | `eslint --fix .` |
| **Prettier** | Code formatting | `prettier --write .` |
| **TypeScript Compiler** | Type checking for TS/JS | `tsc --noEmit` |
| **ts-prune** | Dead code detection | `ts-prune` |
| **SonarQube** | Multi-language code quality | CI/CD integration |

**AI-Powered Code Review (GitHub Integration):**

| Tool | Key Feature |
|------|-------------|
| **GitHub Copilot** | Suggests refactorings, generates tests, explains legacy functions |
| **CodeRabbit** | Full codebase context-aware reviews in PRs |
| **Qodo.ai (Codium)** | AI test generation and quality checks |
| **DeepSource** | Security + performance analysis |
| **Greptile** | Semantic code search across large repos |

### 3. Refactoring Process (Step-by-Step)

> [!IMPORTANT]
> **Always write tests BEFORE refactoring. If no tests exist, create characterization tests first.**

1. **Understand the Intent**
   - Ask: "What problem was this solving?"
   - Review git history: `git log --follow <file>` and `git blame <file>`
   - Document assumptions in code comments

2. **Establish Safety Nets**
   - Run existing tests: `pytest tests/` (Python) or `npm test` (JS)
   - Measure current coverage: `pytest --cov` → target 80%+ for critical paths
   - Add characterization tests if coverage < 60%

3. **Incremental Refactoring (Layer by Layer)**
   - **Phase 1:** Fix linting/formatting (Ruff, Prettier) — *low risk*
   - **Phase 2:** Add type hints (mypy, TypeScript) — *medium risk*
   - **Phase 3:** Extract functions, reduce complexity — *medium risk*
   - **Phase 4:** Replace deprecated APIs with modern equivalents — *high risk*
   - **Phase 5:** Extract to new modules (strangler fig pattern) — *high risk*

4. **Verification Gates**
   - All existing tests pass (`pytest`, `npm test`)
   - No new linting errors (`ruff check`, `eslint`)
   - Code coverage unchanged or improved (`pytest --cov`)
   - Security scan passes (`bandit`, `npm audit`)

5. **Documentation**
   - Update docstrings with modern examples
   - Add inline comments explaining **why**, not **what**
   - Create migration guide if API changes affect users

### 4. Common Legacy Code Patterns (Python)

| Deprecated | Modern Replacement | Reason |
|-----------|-------------------|--------|
| `from collections import MutableMapping` | `from collections.abc import MutableMapping` | Moved in Python 3.3+ |
| `os.path.join(a, b)` | `pathlib.Path(a) / b` | Type-safe, cleaner API |
| `% formatting` | `f"{variable}"` or `str.format()` | More readable, faster |
| `try/except` without context | `with contextlib.suppress(Exception):` | Cleaner error handling |
| `dict.has_key(k)` | `k in dict` | Pythonic, removed in Py3 |
| No type hints | `def func(x: int) -> str:` | Catches bugs at dev time |

### 5. GitHub Pull Request Best Practices (2024-2025)

> [!TIP]
> **Keep PRs small (<400 lines) for faster, more accurate reviews.**

- **Small PRs:** Break large refactors into atomic changes
- **Self-Review First:** Review your own PR before requesting others
- **Automated Checks:** Enforce linting, tests, security scans in CI/CD
- **Clear Descriptions:** Explain **what**, **why**, **impact**, **how tested**
- **Use Draft PRs:** For work-in-progress, get early feedback
- **Feature Flags:** For risky changes, use toggles to A/B test old vs new logic

---

## Output Format

### Provide the following in your review:

1. **Summary of Findings**
   - List deprecated patterns found (with line numbers)
   - Complexity metrics (if applicable)
   - Security vulnerabilities (if any)

2. **Recommended Tooling**
   - Specific commands to run (e.g., `ruff check --fix src/`)
   - Expected output/improvements

3. **Refactoring Plan (Phased)**
   - Phase 1: Low-risk fixes (formatting, linting)
   - Phase 2: Medium-risk (type hints, small extractions)
   - Phase 3: High-risk (API migrations, architectural changes)

4. **Example Code Block**
   - Show **before** and **after** code
   - Include comments explaining the change
   - Demonstrate modern conventions

---

## Example Usage

**User Request:**
> "Can you review `@src/legacy/old_database_connector.py` and suggest how to update it for better performance and maintainability?"

**Agent Response:**

### Summary of Findings
- **Line 23:** Using deprecated `collections.MutableMapping` (should be `collections.abc.MutableMapping`)
- **Line 45-67:** Database connection logic duplicated 3 times (DRY violation)
- **Line 89:** No type hints, making debugging difficult
- **Complexity:** McCabe score of 15 (threshold is 10)

### Recommended Tooling
```bash
# Run modern linters
ruff check --fix src/legacy/old_database_connector.py
mypy src/legacy/old_database_connector.py
bandit -r src/legacy/

# Check complexity
radon cc -s src/legacy/old_database_connector.py
```

### Refactoring Plan
**Phase 1 (Low Risk):**
- Fix import: `collections.MutableMapping` → `collections.abc.MutableMapping`
- Run Ruff to auto-fix formatting/linting

**Phase 2 (Medium Risk):**
- Add type hints to all functions
- Extract duplicated connection logic to `_get_db_connection()` helper

**Phase 3 (High Risk):**
- Replace raw SQL with SQLAlchemy ORM (if applicable)
- Implement connection pooling for performance

### Example Refactoring

**Before (Legacy):**
```python
from collections import MutableMapping  # DEPRECATED

def connect_db(host, port):
    conn = psycopg2.connect(host=host, port=port)  # Duplicated 3 times
    return conn
```

**After (Modern):**
```python
from collections.abc import MutableMapping
from typing import Optional
import psycopg2
from psycopg2 import pool

# Connection pool (shared resource)
_connection_pool: Optional[pool.SimpleConnectionPool] = None

def get_db_connection(host: str, port: int) -> psycopg2.extensions.connection:
    """
    Get a database connection from the pool.

    Args:
        host: Database host (e.g., 'localhost')
        port: Database port (e.g., 5432)

    Returns:
        Active database connection

    Raises:
        psycopg2.OperationalError: If connection fails
    """
    global _connection_pool
    if _connection_pool is None:
        _connection_pool = pool.SimpleConnectionPool(
            minconn=1, maxconn=10, host=host, port=port
        )
    return _connection_pool.getconn()
```

**Key Improvements:**
- ✅ Fixed deprecated import
- ✅ Added type hints (`str`, `int`, `psycopg2.extensions.connection`)
- ✅ Extracted to single function (DRY)
- ✅ Implemented connection pooling (performance)
- ✅ Added comprehensive docstring

---

## Verification Checklist

Before marking refactoring complete:

- [ ] All existing tests pass
- [ ] Linting passes (`ruff check`, `eslint`)
- [ ] Type checking passes (`mypy`, `tsc`)
- [ ] Security scan clean (`bandit`, `npm audit`)
- [ ] Code coverage maintained or improved
- [ ] Documentation updated
- [ ] PR reviewed by at least one peer
- [ ] Manual testing performed (if applicable)

---

## Additional Resources

- [GitHub Copilot for Legacy Code](https://github.blog/2024-01-22-10-unexpected-ways-to-use-github-copilot/)
- [Refactoring Guru (Design Patterns)](https://refactoring.guru/)
- [Python Type Hints Cheat Sheet](https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html)
- [Google's Code Review Best Practices](https://google.github.io/eng-practices/review/)