Fix Bad Practices Skill

Systematically fix bad coding practices identified by audit, following fail-fast principles.

When to Use

Use this skill:

• After running /audit-code-quality
• When fixing specific bad patterns
• During refactoring for code quality
• Autonomously as part of quality workflow

Philosophy: Fail Fast Fixes

When fixing bad practices:

1. Make errors visible - Replace silent failures with loud ones
2. Be specific - Catch only exceptions you can handle
3. Always log - Enable debugging by humans and LLMs
4. Preserve stack traces - Use raise from or raise
5. Test after fixing - Ensure fixes don't break functionality

Fix Patterns

Fix 1: Bare except: pass → Specific Exception + Logging

Before (BAD):

python
1try:
2    do_something()
3except:
4    pass

After (GOOD):

python
1try:
2    do_something()
3except SpecificError as e:
4    logger.exception("Failed to do_something: %s", e)
5    raise

Or if recovery is possible:

python
1try:
2    result = do_something()
3except SpecificError as e:
4    logger.warning("do_something failed, using fallback: %s", e)
5    result = fallback_value  # Only if fallback is valid!

Fix 2: except Exception → Specific Exceptions

Before (BAD):

python
1try:
2    data = parse_json(text)
3except Exception as e:
4    logger.error(e)
5    return None

After (GOOD):

python
1try:
2    data = parse_json(text)
3except json.JSONDecodeError as e:
4    logger.exception("Invalid JSON: %s", e)
5    raise ValueError(f"Cannot parse JSON: {e}") from e
6except FileNotFoundError as e:
7    logger.exception("File not found: %s", e)
8    raise

Fix 3: Silent continue → Explicit Error Handling

Before (BAD):

python
1for item in items:
2    if not item.is_valid():
3        continue
4    process(item)

After (GOOD) - Option A: Fail on first error:

python
1for item in items:
2    if not item.is_valid():
3        raise ValueError(f"Invalid item: {item}")
4    process(item)

After (GOOD) - Option B: Collect errors, report at end:

python
1errors = []
2for item in items:
3    if not item.is_valid():
4        errors.append(f"Invalid item: {item}")
5        continue
6    process(item)
7
8if errors:
9    raise ValueError(f"Processing failed with {len(errors)} invalid items:\n" +
10                     "\n".join(errors))

After (GOOD) - Option C: Log warning if skip is intentional:

python
1for item in items:
2    if not item.is_valid():
3        logger.warning("Skipping invalid item: %s (reason: %s)", item, item.validation_error)
4        continue
5    process(item)

Fix 4: return None on Error → Raise Exception

Before (BAD):

python
1def load_config(path):
2    if not os.path.exists(path):
3        return None
4    with open(path) as f:
5        return json.load(f)

After (GOOD):

python
1def load_config(path: Path) -> dict:
2    """Load configuration from path.
3
4    Raises:
5        FileNotFoundError: If config file doesn't exist
6        json.JSONDecodeError: If config is not valid JSON
7    """
8    if not path.exists():
9        raise FileNotFoundError(f"Config file not found: {path}")
10
11    with open(path) as f:
12        return json.load(f)

Fix 5: Error Return Codes → Exceptions

Before (BAD):

python
1def process_data(data):
2    if not data:
3        return -1, "No data provided"
4    if not validate(data):
5        return -2, "Invalid data"
6    result = transform(data)
7    return 0, result

After (GOOD):

python
1def process_data(data):
2    """Process the data.
3
4    Raises:
5        ValueError: If data is empty or invalid
6    """
7    if not data:
8        raise ValueError("No data provided")
9    if not validate(data):
10        raise ValueError(f"Invalid data: {get_validation_errors(data)}")
11    return transform(data)

Fix 6: Defensive None Checks → Fail Fast

Before (BAD):

python
1def process(data):
2    if data is None:
3        return
4    # ... rest of processing

After (GOOD) - If None is never valid:

python
1def process(data):
2    if data is None:
3        raise ValueError("data cannot be None")
4    # ... rest of processing

After (GOOD) - If function should handle None gracefully:

python
1def process(data: Optional[Data]) -> Optional[Result]:
2    """Process data if provided.
3
4    Args:
5        data: Data to process, or None to skip processing
6
7    Returns:
8        Result if data was processed, None if data was None
9    """
10    if data is None:
11        logger.debug("process() called with None, returning None")
12        return None
13    # ... rest of processing

Fix 7: subprocess Without Error Checking → check=True

Before (BAD):

python
1subprocess.run(["git", "commit", "-m", "message"])

After (GOOD):

python
1try:
2    subprocess.run(
3        ["git", "commit", "-m", "message"],
4        check=True,
5        capture_output=True,
6        text=True
7    )
8except subprocess.CalledProcessError as e:
9    logger.exception("Git commit failed: %s\nstderr: %s", e, e.stderr)
10    raise

Fix 8: Catch-Log-Reraise → Add Context or Remove

Before (BAD) - Adds noise:

python
1try:
2    do_something()
3except SomeError as e:
4    logger.error(f"Error: {e}")
5    raise

After (GOOD) - Option A: Add context:

python
1try:
2    do_something()
3except SomeError as e:
4    logger.exception("Failed during do_something in context X")
5    raise RuntimeError(f"Operation failed in context X") from e

After (GOOD) - Option B: Just let it propagate:

python
1do_something()  # Let caller handle the exception

Adding Proper Logging

Logger Setup

Ensure each module has a logger:

python
1import logging
2
3logger = logging.getLogger(__name__)

Logging Levels Guide

Logging Best Practices

python
1# GOOD - Use exception() in except blocks (includes stack trace)
2except SomeError as e:
3    logger.exception("Operation failed: %s", e)
4
5# GOOD - Use lazy formatting
6logger.debug("Processing item %s of %s", i, total)
7
8# BAD - Eager string formatting
9logger.debug(f"Processing item {i} of {total}")  # Formats even if debug disabled
10
11# GOOD - Include relevant context
12logger.error("Failed to load preset '%s' from %s: %s", preset_id, path, e)
13
14# BAD - Vague messages
15logger.error("Error occurred")

Fix Workflow

Step 1: Run Audit

bash
1# Run audit first
2/audit-code-quality
3
4# Or run grep patterns directly
5grep -rn "except.*:$" --include="*.py" -A1 | grep -B1 "pass$"

Step 2: Categorize Issues

Priority order:

1. except: pass or except Exception: pass (CRITICAL)
2. Overly broad except Exception (HIGH)
3. Silent continuation patterns (MEDIUM)
4. Missing logging (MEDIUM)
5. Return None on error (LOW)

Step 3: Fix Each Issue

For each issue:

1. Understand the intent - Why was error suppressed?
2. Determine correct handling:
   • Should it fail fast? → Raise exception
   • Is recovery possible? → Handle specifically with logging
   • Is it truly optional? → Document and log at DEBUG level
3. Apply fix pattern from above
4. Add/update tests for error conditions
5. Run tests to verify fix

Step 4: Verify Fixes

bash
1# Run tests
2uv run pytest MouseMaster/Testing/Python/ -v
3
4# Run linting
5uv run ruff check .
6
7# Re-run audit to confirm
8/audit-code-quality

Automated Fix Script

For simple patterns, use sed (review changes before committing!):

bash
1#!/bin/bash
2# CAUTION: Review all changes manually!
3
4# Find files with except:pass (for manual review)
5echo "Files needing manual review:"
6grep -rl "except.*:" --include="*.py" | while read f; do
7    if grep -q "pass$" "$f"; then
8        echo "  $f"
9    fi
10done

Note: Automated fixes are risky. Always review manually and run tests.

Exception Handling Decision Tree

Is this a programming error (bug)?
├─ Yes → Let it crash (don't catch)
└─ No → Can user/system recover?
         ├─ Yes → Catch, log, handle gracefully
         └─ No → Catch, log with context, re-raise or wrap

When catching:
├─ Can you name the specific exception?
│   ├─ Yes → Catch that specific type
│   └─ No → Research what exceptions can occur
└─ Is logging added?
    ├─ Yes → Good
    └─ No → Add logger.exception() call

Valid Exception Handling Cases

Not all exception handling is bad. Valid cases include:

1. User Input Validation

python
1try:
2    value = int(user_input)
3except ValueError:
4    logger.warning("Invalid input '%s', prompting user", user_input)
5    show_error_to_user("Please enter a valid number")

2. Optional Feature Degradation

python
1try:
2    import optional_dependency
3    FEATURE_AVAILABLE = True
4except ImportError:
5    logger.info("Optional feature unavailable: optional_dependency not installed")
6    FEATURE_AVAILABLE = False

3. Resource Cleanup

python
1try:
2    resource = acquire_resource()
3    use_resource(resource)
4finally:
5    resource.cleanup()  # Always runs

4. Retry Logic

python
1for attempt in range(max_retries):
2    try:
3        return do_operation()
4    except TransientError as e:
5        logger.warning("Attempt %d failed: %s", attempt + 1, e)
6        if attempt == max_retries - 1:
7            raise
8        time.sleep(backoff)

5. Boundary/API Error Translation

python
1try:
2    result = external_api.call()
3except ExternalAPIError as e:
4    logger.exception("External API failed")
5    raise OurDomainError(f"Service unavailable: {e}") from e

Testing Error Paths

After fixing, add tests for error conditions:

python
1def test_load_config_missing_file():
2    """Test that missing config raises FileNotFoundError."""
3    with pytest.raises(FileNotFoundError, match="Config file not found"):
4        load_config(Path("/nonexistent/path"))
5
6def test_process_invalid_data():
7    """Test that invalid data raises ValueError with details."""
8    with pytest.raises(ValueError, match="Invalid data"):
9        process_data({"bad": "data"})

Commit Message Template

After fixes:

fix: replace exception swallowing with proper error handling

- Remove except:pass in module_name.py
- Add specific exception types with logging
- Add error path tests

Follows fail-fast principle: errors now surface immediately
with full context for debugging.