Refactoring with Functional Core, Imperative Shell
refactoring
Use when user explicitly asks to refactor code, or when test coverage is requested for untested code with side effects. Enforces Functional Core Imperative Shell pattern extraction before any changes.
SKILL.md
Full skill instructions
Refactoring with Functional Core, Imperative Shell
Extract pure functions before refactoring. Never refactor code that mixes logic with side effects.
When to Use
- User asks to "refactor", "consolidate", "extract", or "reduce duplication"
- Test coverage requested for hooks, components, or code with I/O
- Code review reveals logic buried in side-effect-heavy code
Core Pattern
1. ANALYZE → Identify pure logic vs side effects
2. EXTRACT → Move pure logic to utils/ as pure functions
3. TEST → Write output-based tests for extracted functions
4. REFACTOR → Modify imperative shell (now thin wrapper)
Implementation
Step 1: Analyze Before Touching Code
// IDENTIFY in existing code:
// - Pure logic (calculations, transformations, validations)
// - Side effects (API calls, state updates, localStorage, Date.now())
// Example: useCommentSuggestions.ts
// PURE: isCacheValid(entry, currentTime, ttl) → boolean
// IMPURE: localStorage.getItem(), Date.now(), queryClient.setQueryData()
Step 2: Extract Pure Functions
// Before: Logic mixed with side effects
const loadFromCache = (key: string) => {
const cached = localStorage.getItem(key); // side effect
if (!cached) return undefined;
const entry = JSON.parse(cached);
return Date.now() - entry.timestamp <= TTL ? entry.data : undefined; // impure
};
// After: Pure function extracted
export const isCacheValid = (timestamp: number, currentTime: number, ttl: number): boolean =>
currentTime - timestamp <= ttl;
// Imperative shell becomes thin
const loadFromCache = (key: string) => {
const cached = localStorage.getItem(key);
if (!cached) return undefined;
const entry = JSON.parse(cached);
return isCacheValid(entry.timestamp, Date.now(), TTL) ? entry.data : undefined;
};
Step 3: Test Only Pure Functions
describe('isCacheValid', () => {
it('returns true when within TTL', () => {
expect(isCacheValid(1000, 2000, 5000)).toBe(true);
});
it('returns false when TTL exceeded', () => {
expect(isCacheValid(1000, 10000, 5000)).toBe(false);
});
});
Common Mistakes
| Mistake | Why It's Wrong | Do Instead |
|---|---|---|
| Refactor first, test later | No safety net for regressions | Extract + test pure functions first |
| Mock Date.now() in tests | Testing implementation, not behavior | Inject time as parameter |
| Test hooks directly | Requires QueryClient, context mocking | Extract logic, test pure functions |
| Skip analysis step | Miss extraction opportunities | Always analyze pure vs impure first |
Red Flags
Stop and re-analyze if you find yourself:
- Writing
vi.mock()for more than external APIs - Testing a function that calls
useState,useQuery, or Firebase - Unable to test without
renderHook()orQueryClientProvider
