refactor-code-quality

Refactor for Code Quality

After implementing a feature, review the changes to remove code duplication, extract shared patterns, and ensure type safety. This skill enforces the codebase's core principles of modularization and type correctness.

When to Use

• After implementing any new feature - Review all changed files
• After fixing bugs - Check if the fix introduced duplication
• During code review - Identify patterns that should be shared
• Proactively - Periodically audit the codebase for accumulated duplication

Process

Step 1: Identify All Changed Files on Branch

# See all files changed on this branch compared to main
git diff origin/main --name-only

# See the full diff of all changes on the branch
git diff origin/main

Step 2: Analyze for Code Duplication

Look for these patterns in the changed code:

2a. Repeated Logic Blocks

Search for similar code patterns:

# Look for similar function structures
rg "function.*\(" --type ts -l

# Look for repeated patterns (example: similar conditionals)
rg "if.*===.*null" --type ts -C 2

Signs of duplication:

• Multiple functions with similar structure but different details
• Copy-pasted code blocks with minor modifications
• Switch statements or if-chains that could be data-driven
• Similar error handling patterns repeated across files

2b. Parallel Logic Instead of Shared Utilities

Check if new code duplicates existing helpers:

# Find existing utility functions in src/
rg "^export (function|const)" src/ --type ts

# Check for similar patterns to what you just added
rg "<pattern-from-your-code>" src/

Questions to ask:

• Does a helper already exist that does something similar?
• Can an existing helper be extended to cover the new case?
• Should this logic be extracted to a shared utility?

Step 3: Analyze for Type Safety Issues

3a. Find any Types

# Search for any types in changed files
rg ": any" --type ts
rg "as any" --type ts
rg "<any>" --type ts

Fix by:

• Defining proper interfaces or type aliases
• Using generics for flexible but type-safe code
• Using unknown with type guards when type is truly unknown

Note: jscodeshift's AST types can make some patterns difficult to type precisely. In cases where jscodeshift's type definitions are incomplete or overly broad, a well-placed type assertion may be acceptable if it improves code clarity. Prefer narrowing with type guards when possible, but don't contort the code just to avoid a single assertion in AST manipulation code.

3b. Find Type Assertions

# Search for type assertions
rg " as [A-Z]" --type ts
rg "(<[A-Z][a-zA-Z]*>)" --type ts

# Search for non-null assertions
rg "!\." --type ts
rg "!\[" --type ts
rg "!;" --type ts

Fix by:

• Adding proper null checks with early returns
• Using optional chaining (?.) where appropriate
• Narrowing types with type guards
• Fixing the source of the type uncertainty

3c. Find Linter Warning Suppression Hacks

# Search for void used to suppress unused variable warnings
rg "^\s*void \w+;" --type ts

Never use void variable; to suppress unused variable warnings. This is a code smell that hides the real problem. Instead:

• Remove the parameter if it's not needed
• Actually use it if it should be doing something

Step 4: Refactor to Remove Duplication

4a. Extract Shared Functions

When you find duplicated logic:

1. Identify the common pattern - What's the same across all instances?
2. Identify the variations - What differs? These become parameters.
3. Create or extend a helper - Place it in the appropriate module:
   • src/internal/ for internal utilities
   • Near the code that uses it if only used in one area

4b. Prefer Shared Context Objects Over Large Argument Bags

When helper functions need many of the same parameters (e.g., j, filePath, warnings, parseExpr, resolveValue), avoid constructing large inline argument objects at each call site. Instead, pass a shared context object plus only the call-specific arguments:

Bad — duplicates the same fields at every call site and bloats the type:

tryResolveBlockLevel({
  j,
  filePath,
  warnings,
  parseExpr,
  resolveValue,
  resolveCall,
  resolveImportInScope,
  resolverImports,
  handlerContext,
  // ...10 more shared fields
  conditional, // ← the only call-specific field
});

Good — pass the shared context and only spell out what's unique:

tryResolveBlockLevel(ctx, { conditional });

Where ctx is a commonly defined context type (e.g., DeclProcessingState, CssHelperConditionalContext) that bundles the shared fields. The helper's type can use Pick<Context, ...> if it only needs a subset.

This pattern:

• Eliminates repetitive field listings at call sites
• Makes it obvious which arguments are call-specific vs shared
• Keeps types in sync automatically (use Pick from the source type)
• Reduces the chance of forgetting to pass a new field when the context grows

4c. Use Data-Driven Approaches

Replace repetitive conditionals (switch statements, if-chains) with lookup tables or maps.

Step 5: Ensure Proper Type Definitions

• Replace any types with proper interfaces, type aliases, or generics where feasible
• Replace type assertions (using "as") with type guards that narrow types safely
• Replace non-null assertions (the ! operator) with proper null checks and early returns

Note: Some jscodeshift AST patterns are difficult to type precisely. Accept minimal, well-placed assertions when they improve clarity over convoluted type gymnastics.

Step 6: Verify Changes

Run the full validation suite:

pnpm check

This ensures:

• No type errors introduced
• No lint violations
• All tests still pass
• No dead code created

Step 7: Review Export Organization

Ensure exports are at the top of files (after imports), with non-exported helpers further down.

Step 8: Commit Refactoring Separately

Keep refactoring commits separate from feature commits for cleaner history:

git add <refactored-files>
git commit -m "refactor: extract shared helper for <pattern>

- Reduces duplication in <files>
- Adds proper types for <area>
"
git push

Checklist

Before considering the refactoring complete:

• No duplicated logic blocks across files
• Similar patterns use shared helpers
• Helper functions use shared context objects instead of large argument bags
• No unnecessary "any" types (some jscodeshift patterns may require them)
• Type assertions minimized and justified (AST manipulation may need some)
• No unnecessary non-null assertions (the ! operator)
• No void variable; hacks to suppress linter warnings
• Exports are at the top of each file
• pnpm check passes
• Helper functions have descriptive names