Safe refactoring patterns with step-by-step sequences: extract method/class, decompose conditional, replace magic number, introduce parameter object. Use when cleaning up code without changing behavior.
From sde-code-qualitynpx claudepluginhub chavangorakh1999/sde-skills --plugin sde-code-qualityThis skill uses the workspace's default tool permissions.
Designs and optimizes AI agent action spaces, tool definitions, observation formats, error recovery, and context for higher task completion rates.
Manages GitHub repositories using gh CLI: triages issues by type/priority, handles PR reviews/CI checks/merges, debugs CI/CD failures, prepares releases, monitors security/stale items.
Compares coding agents like Claude Code and Aider on custom YAML-defined codebase tasks using git worktrees, measuring pass rate, cost, time, and consistency.
Refactoring changes code structure without changing behavior. The key word is safe: each step must leave the tests green. Refactor in small, verifiable moves — not one giant rewrite.
Code to refactor: $ARGUMENTS
Never refactor and add features simultaneously. Two separate commits:
Before touching code, you need tests that catch regressions.
// If tests don't exist, write characterization tests first:
// Characterization test = "document what the code currently does"
// (even if what it does is weird or wrong — that's a separate fix)
describe('UserRegistrationService (characterization)', () => {
it('currently hashes password with MD5 (known bug — do not fix here)', () => {
const service = new UserRegistrationService();
const user = service.register({ email: 'a@b.com', password: 'pass123' });
expect(user.passwordHash).toMatch(/^[a-f0-9]{32}$/); // MD5 format
});
// ... document other current behaviors
});
// Run tests before starting. Note which pass. Refactoring is complete only
// when all previously-passing tests still pass.
The most common refactoring. When a function does more than one thing, extract the parts.
// Before: one big function doing many things
async function processOrder(orderId) {
const order = await Order.findById(orderId);
if (!order) throw new Error('Order not found');
// Validate order
if (order.items.length === 0) throw new Error('Empty order');
if (order.total < 0) throw new Error('Invalid total');
const hasStock = await Promise.all(order.items.map(item =>
Inventory.check(item.productId, item.quantity)
));
if (hasStock.some(s => !s)) throw new Error('Out of stock');
// Charge payment
const paymentIntent = await stripe.paymentIntents.create({
amount: Math.round(order.total * 100),
currency: 'usd',
customer: order.customerId
});
// Update order
order.status = 'paid';
order.paymentIntentId = paymentIntent.id;
order.paidAt = new Date();
await order.save();
// Send confirmation
await emailService.send(order.customerEmail, 'order-confirmation', { order });
}
// After: extract each responsibility into a named method
async function processOrder(orderId) {
const order = await findOrder(orderId);
await validateOrder(order);
const paymentIntent = await chargeOrder(order);
await markOrderPaid(order, paymentIntent);
await sendConfirmation(order);
}
async function findOrder(orderId) {
const order = await Order.findById(orderId);
if (!order) throw new NotFoundError(`Order ${orderId} not found`);
return order;
}
async function validateOrder(order) {
if (order.items.length === 0) throw new ValidationError('Order has no items');
if (order.total < 0) throw new ValidationError('Order total is invalid');
const stockResults = await Promise.all(
order.items.map(item => Inventory.check(item.productId, item.quantity))
);
if (stockResults.some(inStock => !inStock)) {
throw new ValidationError('One or more items are out of stock');
}
}
// ... chargeOrder, markOrderPaid, sendConfirmation each in ~5-10 lines
Steps:
When a class has grown multiple unrelated responsibilities.
// Before: UserService doing too many things
class UserService {
async register(data) { ... }
async login(email, password) { ... }
async updateProfile(userId, data) { ... }
async forgotPassword(email) { ... } // email concern
async resetPassword(token, password) { ... } // email concern
async sendVerificationEmail(userId) { ... } // email concern
async verifyEmail(token) { ... } // email concern
}
// After: separate auth concerns from profile concerns from email concerns
class UserService {
async register(data) { ... } // core user lifecycle
async updateProfile(userId, data) { ... }
}
class AuthService {
async login(email, password) { ... }
async resetPassword(token, password) { ... }
}
class EmailVerificationService {
async sendVerification(userId) { ... }
async verify(token) { ... }
async forgotPassword(email) { ... }
}
Steps:
Complex if/else logic into named predicates or strategy pattern.
// Before: dense conditional logic
function calculateDiscount(user, order) {
if (user.subscriptionTier === 'premium' && order.total > 100 && !order.hasDigitalItems) {
return order.total * 0.15;
} else if (user.loyaltyPoints > 1000 && order.total > 50) {
return order.total * 0.10;
} else if (user.referralCode && order.isFirstOrder) {
return order.total * 0.20;
}
return 0;
}
// After: named predicates + early returns + clear structure
function calculateDiscount(user, order) {
if (qualifiesForPremiumDiscount(user, order)) return order.total * 0.15;
if (qualifiesForLoyaltyDiscount(user, order)) return order.total * 0.10;
if (qualifiesForReferralDiscount(user, order)) return order.total * 0.20;
return 0;
}
function qualifiesForPremiumDiscount(user, order) {
return user.subscriptionTier === 'premium'
&& order.total > 100
&& !order.hasDigitalItems;
}
function qualifiesForLoyaltyDiscount(user, order) {
return user.loyaltyPoints > 1000 && order.total > 50;
}
function qualifiesForReferralDiscount(user, order) {
return Boolean(user.referralCode) && order.isFirstOrder;
}
When a function takes many related parameters, group them.
// Before: many parameters
async function createReport(startDate, endDate, userId, tenantId, format, includeArchived) {
// ...
}
// After: parameter object
async function createReport({ startDate, endDate, userId, tenantId, format, includeArchived = false }) {
// ...
}
// Callers become readable:
await createReport({
startDate: new Date('2024-01-01'),
endDate: new Date('2024-12-31'),
userId: currentUser.id,
tenantId: currentTenant.id,
format: 'pdf'
// includeArchived defaults to false
});
// Before
if (retries > 3) { ... }
if (user.tier === 2) { ... }
setTimeout(fn, 300000);
// After
const MAX_RETRY_ATTEMPTS = 3;
const USER_TIERS = Object.freeze({ FREE: 0, BASIC: 1, PREMIUM: 2 });
const SESSION_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes, expressed as math
if (retries > MAX_RETRY_ATTEMPTS) { ... }
if (user.tier === USER_TIERS.PREMIUM) { ... }
setTimeout(fn, SESSION_TIMEOUT_MS);
For any significant refactoring:
Never accumulate multiple refactoring moves before testing. If tests break, you know exactly which move caused it.
## Refactoring Plan: [File/Class Name]
### Current Issues
[Code smells identified, with references to specific line numbers]
### Refactoring Steps
#### Step 1: [Refactoring name]
Before:
```js
[original code]
After:
[refactored code]
Why: [rationale]
[What tests to write or verify before starting]
[What could break, how to detect it]