Refactoring in Practice
I was reviewing a PR. The developer had added a feature and, in the same commit, refactored half the file. The feature worked. The refactor introduced a subtle bug that only appeared when a specific optional parameter was passed.
We reverted the whole PR. The feature was delayed by two days. The refactor was lost.
The lesson: refactoring in practice isn't just about technique. It's about when to do it, how to sell it, and how to measure it.
When to Refactor: Opportunistic vs. Planned
Opportunistic Refactoring
Opportunistic refactoring happens when you're already in the code for another reason. You're fixing a bug or adding a feature. You notice a smell. You fix it as part of the same work.
Rule: The refactor must be smaller than the change you're already making.
If you're adding a 20-line feature and the "quick refactor" becomes 200 lines, stop. Ship the feature. Open a separate ticket for the refactor.
// You're adding validation for email format
// Opportunistic: Extract the validation while you're here
function createUser(data: UserInput) {
if (!isValidEmail(data.email)) {
throw new Error('Invalid email');
}
// ... rest of function
}
// NOT opportunistic: "While I'm here, let me refactor the entire user module"
Planned Refactoring
Planned refactoring is work scheduled and scoped. It has a ticket. It has acceptance criteria. It's estimated.
When to plan refactoring:
- A module is blocking multiple features
- Bug rate in an area is high
- Onboarding new developers is slow because of a specific area
- You're about to add major functionality to messy code
When not to plan it:
- "The code could be cleaner" (vague)
- "We should modernize" (no clear benefit)
- "I don't like how it looks" (aesthetic, not functional)
The Rule of Three
A heuristic: refactor on the third time. The first time you touch code, you might not understand it well enough. The second time, you see the pattern. The third time, you're confident—refactor then.
Refactoring During Code Review
Code review is a negotiation. Refactoring in a PR can trigger pushback: "Scope creep." "Just ship the feature." "We'll fix it later."
Make Refactoring Reviewable
Separate commits. Feature in one commit, refactor in another. Reviewers can approve the feature and scrutinize the refactor separately.
git log --oneline
a1b2c3d Add email validation (feature)
d4e5f6g Extract validation helpers (refactor)
Call it out. In the PR description: "This PR adds X. Commit 2 is a refactor of Y to support it. Happy to split into separate PR if preferred."
Keep it proportional. A 10-line feature with 100 lines of refactoring will get rejected. A 50-line feature with 20 lines of refactoring is reasonable.
When to Request Refactoring in Review
As a reviewer, request refactoring when:
- The new code worsens an existing smell
- The change would be simpler with a small extraction
- There's clear duplication with existing code
Don't request it when:
- It's purely stylistic
- It would significantly delay the PR
- The author would need to understand a large unfamiliar area
Convincing Stakeholders
"Can we take two weeks to refactor the payment module?"
The answer is often no. Not because refactoring is bad, but because the request is vague. Stakeholders hear "we want to stop shipping features and clean up."
Frame in Business Terms
| Instead of... | Say... |
|---|---|
| "We need to refactor" | "This module has caused 3 production incidents this quarter. A focused cleanup will reduce that." |
| "The code is messy" | "Adding the new pricing feature will take 3 weeks in the current state. With a 1-week cleanup first, it'll take 2 weeks total." |
| "Technical debt" | "Every change to this area has a 50% chance of introducing a bug. We're spending more time fixing regressions than building features." |
Propose a Trade
"Give us 3 days. We'll extract the validation layer and add tests. After that, the next two features in this area will ship faster." Concrete. Time-bound. With a payoff.
Use Incidents as Leverage
After a production bug: "The root cause was unclear error handling in this module. Can we schedule a day to add tests and clarify the flow? It'll reduce the chance of similar bugs."
Stakeholders understand risk. Frame refactoring as risk reduction.
Measuring Improvement
"How do we know the refactor helped?"
Before/After Metrics
| Metric | What to capture |
|---|---|
| Cyclomatic complexity | Lower is better. Many tools can report this. |
| Lines of code | Sometimes fewer is better; sometimes extraction adds lines but clarity. Use judgment. |
| Test coverage | Did you add tests? What's the coverage of the refactored area? |
| Bug rate | Track bugs in the module before and after. |
| Time to implement | How long did the next feature in this area take? |
The Simplest Measure
Can the next developer understand it faster? Ask someone who didn't do the refactor to explain the code. Time them. If it's faster, you've improved things.
Real Refactoring Case Study
A team had a 400-line function that processed orders. It validated input, calculated totals, applied discounts, charged payment, updated inventory, sent emails, and logged analytics. One function. No tests.
Step 1: Characterization Tests
They added tests that captured current behavior. Input X produces output Y. No assertions about "correctness"—just "this is what it does now."
// Characterization test - documents current behavior
it('processes order with standard shipping', () => {
const result = processOrder(mockOrder, mockUser);
expect(result.total).toBe(10999); // Cents - what it does now
expect(result.status).toBe('charged');
});
Step 2: Extract One Piece
They extracted validation into validateOrder(). One extraction. Run tests. Commit.
Step 3: Repeat
Over two weeks, they extracted:
validateOrder()calculateOrderTotal()applyDiscounts()chargePayment()fulfillOrder()notifyCustomer()
Each extraction was one PR. Each PR had tests. The original processOrder became an orchestrator:
function processOrder(order: Order, user: User): OrderResult {
validateOrder(order);
const total = calculateOrderTotal(order);
applyDiscounts(order, total);
chargePayment(order, user);
fulfillOrder(order);
notifyCustomer(user, order);
return { status: 'charged', total };
}
Step 4: Document the Transformation
They added a brief comment at the top of the file: "Refactored from monolithic processOrder 2024-03. See PR #423–430 for history."
Future developers could understand why the structure existed.
What They Didn't Do
- They didn't rewrite the logic
- They didn't change behavior
- They didn't do it all in one PR
- They didn't skip tests
The Refactoring Habit
Refactoring in practice is a habit, not a project. Every PR leaves the code a little cleaner. Every bug fix considers whether the bug was a symptom of a deeper smell. Every new feature asks: "Am I making this better or worse?"
The goal isn't perfect code. It's code that's easier to change tomorrow than it is today.
Key insight: Refactor opportunistically when the change is small and proportional. Plan refactoring when a module is blocking progress. Separate refactoring commits in PRs. Convince stakeholders with business impact, not aesthetics. Measure improvement with before/after metrics. Real refactoring is incremental: one extraction, one test, one commit at a time.