Refactoring with Confidence — Safe Transformation Patterns

1// BEFORE refactoring — working but unreadable:
2export function calc(p: number, q: number, r: string, s: boolean): number {
3  let x = 0;
4  if (r === 'Y') { x = p * 0.1; }
5  if (r === 'M') { x = p * 0.05; }
6  if (s) { x = x * 1.1; }
7  return (p - x) * (q / 100 + 1);
8}
9
10// We have 8 tests covering this function. They all pass. Now we can refactor.
11
12// STEP 1: Rename parameters (using IDE rename refactoring)
13// VS Code: F2 on the variable name
14// Rename p → price, q → taxRate, r → couponType, s → isPremiumCustomer
15export function calc(
16  price: number,
17  taxRate: number,
18  couponType: string,
19  isPremiumCustomer: boolean
20): number {
21  let x = 0;
22  if (couponType === 'Y') { x = price * 0.1; }
23  if (couponType === 'M') { x = price * 0.05; }
24  if (isPremiumCustomer) { x = x * 1.1; }
25  return (price - x) * (taxRate / 100 + 1);
26}
27// Run tests: PASS ✅ — pure rename, no behaviour change
28
29// STEP 2: Rename function
30export function calculateOrderTotal(
31  price: number,
32  taxRate: number,
33  couponType: string,
34  isPremiumCustomer: boolean
35): number { ... }
36// Run tests: PASS ✅
37
38// STEP 3: Rename local variable and extract
39let discount = 0;
40if (couponType === 'Y') { discount = price * 0.1; }
41if (couponType === 'M') { discount = price * 0.05; }
42if (isPremiumCustomer) { discount = discount * 1.1; }
43const discountedPrice = price - discount;
44return discountedPrice * (taxRate / 100 + 1);
45// Run tests: PASS ✅
46
47// After 3 small refactoring steps, the function is readable
48// without a single test changing

1// BEFORE: long function with inline logic
2export async function processCheckout(cart: Cart, user: User): Promise<Order> {
3  // Validate cart
4  if (cart.items.length === 0) throw new Error('Cart is empty');
5  for (const item of cart.items) {
6    if (item.qty <= 0) throw new Error(`Invalid quantity for ${item.id}`);
7    if (item.price < 0) throw new Error(`Invalid price for ${item.id}`);
8  }
9
10  // Calculate totals
11  const subtotal = cart.items.reduce((sum, i) => sum + i.price * i.qty, 0);
12  const discount = cart.coupon ? subtotal * 0.1 : 0;
13  const taxableAmount = subtotal - discount;
14  const tax = taxableAmount * 0.08;
15  const total = taxableAmount + tax;
16
17  // Create order record
18  const order = await db.orders.create({
19    userId: user.id,
20    items: cart.items,
21    subtotal,
22    discount,
23    tax,
24    total,
25    status: 'pending',
26  });
27
28  return order;
29}
30
31// AFTER: extract each section (IDE: select code block → Extract Function)
32function validateCart(cart: Cart): void {
33  if (cart.items.length === 0) throw new Error('Cart is empty');
34  for (const item of cart.items) {
35    if (item.qty <= 0) throw new Error(`Invalid quantity for ${item.id}`);
36    if (item.price < 0) throw new Error(`Invalid price for ${item.id}`);
37  }
38}
39
40interface OrderTotals {
41  subtotal: number;
42  discount: number;
43  tax: number;
44  total: number;
45}
46
47function calculateOrderTotals(cart: Cart): OrderTotals {
48  const subtotal = cart.items.reduce((sum, i) => sum + i.price * i.qty, 0);
49  const discount = cart.coupon ? subtotal * 0.1 : 0;
50  const taxableAmount = subtotal - discount;
51  const tax = taxableAmount * 0.08;
52  return { subtotal, discount, tax, total: taxableAmount + tax };
53}
54
55export async function processCheckout(cart: Cart, user: User): Promise<Order> {
56  validateCart(cart);
57  const totals = calculateOrderTotals(cart);
58  return db.orders.create({ userId: user.id, items: cart.items, ...totals, status: 'pending' });
59}
60// All original tests pass ✅
61// validateCart and calculateOrderTotals are also independently testable now

1// SMELL 1: Long Function (> 20 lines)
2// Fix: Extract Function
3
4// SMELL 2: Duplicate Code — same logic appears in two places
5function calculateStudentDiscount(price: number, credits: number): number {
6  if (credits >= 100) return price * 0.2;
7  if (credits >= 50) return price * 0.1;
8  return 0;
9}
10function calculateInstructorDiscount(price: number, credits: number): number {
11  if (credits >= 100) return price * 0.2; // DUPLICATE
12  if (credits >= 50) return price * 0.1;  // DUPLICATE
13  return 0;
14}
15// Fix: Extract a shared function calculateCreditDiscount(price, credits)
16
17// SMELL 3: Primitive Obsession — using string/number where a type would clarify
18function setUserRole(userId: string, role: string): void {
19  // role can be 'admin', 'instructor', 'student' — but any string compiles
20}
21// Fix:
22type UserRole = 'admin' | 'instructor' | 'student';
23function setUserRole(userId: string, role: UserRole): void { }
24
25// SMELL 4: Long Parameter List (> 4 parameters)
26function createCourse(
27  title: string,
28  description: string,
29  price: number,
30  level: string,
31  instructorId: string,
32  published: boolean,
33  tags: string[]
34): Course { }
35// Fix: introduce a parameter object
36interface CreateCourseInput {
37  title: string;
38  description: string;
39  price: number;
40  level: CourseLevel;
41  instructorId: string;
42  published?: boolean;
43  tags?: string[];
44}
45function createCourse(input: CreateCourseInput): Course { }
46
47// SMELL 5: Feature Envy — a method uses another class's data more than its own
48class OrderSummary {
49  format(order: Order): string {
50    // Uses order.user.name, order.user.email, order.user.address.city...
51    // This method belongs in Order or User, not OrderSummary
52    return `${order.user.name} <${order.user.email}> — ${order.user.address.city}`;
53  }
54}
55// Fix: move the method to Order

1// SAFE: pure structural changes (no logic change)
2// - Rename variable, function, class
3// - Extract function (no logic change, just moves code)
4// - Inline variable (remove unnecessary temporary)
5// - Move function to a different module
6// - Add type annotations
7// - Remove dead code (code the tests show is unreachable)
8
9// RISKY: combines structure change with logic change — avoid!
10
11// RISKY EXAMPLE:
12// Original:
13function getDiscount(tier: string): number {
14  if (tier === 'gold') return 0.25;
15  if (tier === 'silver') return 0.15;
16  return 0.05;
17}
18
19// 'Refactoring' that secretly changes behaviour:
20function getDiscount(tier: string): number {
21  const discounts: Record<string, number> = {
22    gold: 0.25,
23    silver: 0.15,
24    bronze: 0.10, // ADDED — was 0.05 before
25  };
26  return discounts[tier] ?? 0; // returns 0 for unknown tiers instead of 0.05
27}
28// This is NOT a refactoring — it changes the return value for bronze and unknown tiers
29// Tests SHOULD catch this if they cover the bronze and unknown-tier cases
30// If tests do not catch it, the tests are incomplete
31
32// RULE: if you find yourself changing test assertions during a 'refactoring',
33// you are changing behaviour, not refactoring. Stop.
34// Write a separate commit for the behaviour change, then a separate refactoring.

1// Scenario: replacing a legacy search implementation with a new one
2// Both must work simultaneously during the transition
3
4// Step 1: Create new implementation alongside old
5export class LegacySearchService {
6  search(query: string): SearchResult[] { /* old implementation */ }
7}
8
9export class NewSearchService {
10  async search(query: string): Promise<SearchResult[]> { /* new implementation */ }
11}
12
13// Step 2: Route through a facade that delegates to one or the other
14const USE_NEW_SEARCH = process.env['FEATURE_NEW_SEARCH'] === 'true';
15
16export class SearchFacade {
17  private legacy = new LegacySearchService();
18  private newService = new NewSearchService();
19
20  async search(query: string): Promise<SearchResult[]> {
21    if (USE_NEW_SEARCH) {
22      return this.newService.search(query);
23    }
24    return Promise.resolve(this.legacy.search(query));
25  }
26}
27
28// Tests for the facade cover both paths:
29test('routes to new service when feature flag is enabled', async () => {
30  process.env['FEATURE_NEW_SEARCH'] = 'true';
31  const facade = new SearchFacade();
32  const spy = jest.spyOn(facade['newService'], 'search').mockResolvedValue([]);
33  await facade.search('typescript');
34  expect(spy).toHaveBeenCalledWith('typescript');
35});
36
37// Step 3: When new service is fully tested and deployed, remove legacy:
38// - Remove LegacySearchService
39// - Remove the feature flag branch from SearchFacade
40// - Tests still pass (they tested the behaviour, not which class)