refactor

# Refactor ## Overview Improve code structure and readability without changing external behavior. Refactoring is gradual evolution, not revolution. Use this for improving existing code, not rewriting from scratch. ## When to Use Use this skill when: - Code is hard to understand or maintain - Functions/classes are too large - Code smells need addressing - Adding features is difficult due to code structure - User asks "clean up this code", "refactor this", "improve this" --- ## Refactoring Principles ### The Golden Rules 1. **Behavior is preserved** - Refactoring doesn't change what the code does, only how 2. **Small steps** - Make tiny changes, test after each 3. **Version control is your friend** - Commit before and after each safe state 4. **Tests are essential** - Without tests, you're not refactoring, you're editing 5. **One thing at a time** - Don't mix refactoring with feature changes ### When NOT to Refactor ``` - Code that works and won't change again (if it ain't broke...) - Critical production code without tests (add tests first) - When you're under a tight deadline - "Just because" - need a clear purpose ``` --- ## Common Code Smells & Fixes ### 1. Long Method/Function ```diff # BAD: 200-line function that does everything - async function processOrder(orderId) { - // 50 lines: fetch order - // 30 lines: validate order - // 40 lines: calculate pricing - // 30 lines: update inventory - // 20 lines: create shipment - // 30 lines: send notifications - } # GOOD: Broken into focused functions + async function processOrder(orderId) { + const order = await fetchOrder(orderId); + validateOrder(order); + const pricing = calculatePricing(order); + await updateInventory(order); + const shipment = await createShipment(order); + await sendNotifications(order, pricing, shipment); + return { order, pricing, shipment }; + } ``` ### 2. Duplicated Code ```diff # BAD: Same logic in multiple places - function calculateUserDiscount(user) { - if (user.membership === 'gold') return user.total * 0.2; - if (user.membership === 'silver') return user.total * 0.1; - return 0; - } - - function calculateOrderDiscount(order) { - if (order.user.membership === 'gold') return order.total * 0.2; - if (order.user.membership === 'silver') return order.total * 0.1; - return 0; - } # GOOD: Extract common logic + function getMembershipDiscountRate(membership) { + const rates = { gold: 0.2, silver: 0.1 }; + return rates[membership] || 0; + } + + function calculateUserDiscount(user) { + return user.total * getMembershipDiscountRate(user.membership); + } + + function calculateOrderDiscount(order) { + return order.total * getMembershipDiscountRate(order.user.membership); + } ``` ### 3. Large Class/Module ```diff # BAD: God object that knows too much - class UserManager { - createUser() { /* ... */ } - updateUser() { /* ... */ } - deleteUser() { /* ... */ } - sendEmail() { /* ... */ } - generateReport() { /* ... */ } - handlePayment() { /* ... */ } - validateAddress() { /* ... */ } - // 50 more methods... - } # GOOD: Single responsibility per class + class UserService { + create(data) { /* ... */ } + update(id, data) { /* ... */ } + delete(id) { /* ... */ } + } + + class EmailService { + send(to, subject, body) { /* ... */ } + } + + class ReportService { + generate(type, params) { /* ... */ } + } + + class PaymentService { + process(amount, method) { /* ... */ } + } ``` ### 4. Long Parameter List ```diff # BAD: Too many parameters - function createUser(email, password, name, age, address, city, country, phone) { - /* ... */ - } # GOOD: Group related parameters + interface UserData { + email: string; + password: string; + name: string; + age?: number; + address?: Address; + phone?: string; + } + + function createUser(data: UserData) { + /* ... */ + } # EVEN BETTER: Use builder pattern for complex construction + const user = UserBuilder + .email('test@example.com') + .password('secure123') + .name('Test User') + .address(address) + .build(); ``` ### 5. Feature Envy ```diff # BAD: Method that uses another object's data more than its own - class Order { - calculateDiscount(user) { - if (user.membershipLevel === 'gold') { + return this.total * 0.2; + } + if (user.accountAge > 365) { + return this.total * 0.1; + } + return 0; + } + } # GOOD: Move logic to the object that owns the data + class User { + getDiscountRate(orderTotal) { + if (this.membershipLevel === 'gold') return 0.2; + if (this.accountAge > 365) return 0.1; + return 0; + } + } + + class Order { + calculateDiscount(user) { + return this.total * user.getDiscountRate(this.total); + } + } ``` ### 6. Primitive Obsession ```diff # BAD: Using primitives for domain concepts - function sendEmail(to, subject, body) { /* ... */ } - sendEmail('user@example.com', 'Hello', '...'); - function createPhone(country, number) { - return `${country}-${number}`; - } # GOOD: Use domain types + class Email { + private constructor(public readonly value: string) { + if (!Email.isValid(value)) throw new Error('Invalid email'); + } + static create(value: string) { return new Email(value); } + static isValid(email: string) { return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email); } + } + + class PhoneNumber { + constructor( + public readonly country: string, + public readonly number: string + ) { + if (!PhoneNumber.isValid(country, number)) throw new Error('Invalid phone'); + } + toString() { return `${this.country}-${this.number}`; } + static isValid(country: string, number: string) { /* ... */ } + } + + // Usage + const email = Email.create('user@example.com'); + const phone = new PhoneNumber('1', '555-1234'); ``` ### 7. Magic Numbers/Strings ```diff # BAD: Unexplained values - if (user.status === 2) { /* ... */ } - const discount = total * 0.15; - setTimeout(callback, 86400000); # GOOD: Named constants + const UserStatus = { + ACTIVE: 1, + INACTIVE: 2, + SUSPENDED: 3 + } as const; + + const DISCOUNT_RATES = { + STANDARD: 0.1, + PREMIUM: 0.15, + VIP: 0.2 + } as const; + + const ONE_DAY_MS = 24 * 60 * 60 * 1000; + + if (user.status === UserStatus.INACTIVE) { /* ... */ } + const discount = total * DISCOUNT_RATES.PREMIUM; + setTimeout(callback, ONE_DAY_MS); ``` ### 8. Nested Conditionals ```diff # BAD: Arrow code - function process(order) { - if (order) { - if (order.user) { - if (order.user.isActive) { - if (order.total > 0) { - return processOrder(order); + } else { + return { error: 'Invalid total' }; + } + } else { + return { error: 'User inactive' }; + } + } else { + return { error: 'No user' }; + } + } else { + return { error: 'No order' }; + } + } # GOOD: Guard clauses / early returns + function process(order) { + if (!order) return { error: 'No order' }; + if (!order.user) return { error: 'No user' }; + if (!order.user.isActive) return { error: 'User inactive' }; + if (order.total <= 0) return { error: 'Invalid total' }; + return processOrder(order); + } # EVEN BETTER: Using Result type + function process(order): Result<ProcessedOrder, Error> { + return Result.combine([ + validateOrderExists(order), + validateUserExists(order), + validateUserActive(order.user), + validateOrderTotal(order) + ]).flatMap(() => processOrder(order)); + } ``` ### 9. Dead Code ```diff # BAD: Unused code lingers - function oldImplementation() { /* ... */ } - const DEPRECATED_VALUE = 5; - import { unusedThing } from './somewhere'; - // Commented out code - // function oldCode() { /* ... */ } # GOOD: Remove it + // Delete unused functions, imports, and commented code + // If you need it again, git history has it ``` ### 10. Inappropriate Intimacy ```diff # BAD: One class reaches deep into another - class OrderProcessor { - process(order) { - order.user.profile.address.street; // Too intimate - order.repository.connection.config; // Breaking encapsulation + } + } # GOOD: Ask, don't tell + class OrderProcessor { + process(order) { + order.getShippingAddress(); // Order knows how to get it + order.save(); // Order knows how to save itself + } + } ``` --- ## Extract Method Refactoring ### Before and After ```diff # Before: One long function - function printReport(users) { - console.log('USER REPORT'); - console.log('============'); - console.log(''); - console.log(`Total users: ${users.length}`); - console.log(''); - console.log('ACTIVE USERS'); - console.log('------------'); - const active = users.filter(u => u.isActive); - active.forEach(u => { - console.log(`- ${u.name} (${u.email})`); - }); - console.log(''); - console.log(`Active: ${active.length}`); - console.log(''); - console.log('INACTIVE USERS'); - console.log('--------------'); - const inactive = users.filter(u => !u.isActive); - inactive.forEach(u => { - console.log(`- ${u.name} (${u.email})`); - }); - console.log('');