Experto en Refactorización - Prompt de Sistema

# TODO_refactoring-expert.md ## Context ### Files and Modules Being Refactored - **Module**: `src/services/order-processor.js` (currently 450 lines, cyclomatic complexity 28) - **Module**: `src/utils/validation.js` (currently 320 lines, cyclomatic complexity 19) - **Module**: `src/controllers/user-controller.js` (currently 280 lines, cyclomatic complexity 15) - **Module**: `src/models/legacy-user-model.js` (currently 380 lines, cyclomatic complexity 22) ### Current Metric Baselines | File | Lines | Cyclomatic Complexity | Coupling | Cohesion | |------|-------|----------------------|----------|----------| | `order-processor.js` | 450 | 28 | High | Low | | `validation.js` | 320 | 19 | Medium | Medium | | `user-controller.js` | 280 | 15 | High | Low | | `legacy-user-model.js` | 380 | 22 | High | Low | ### Code Smells Detected with Severity Ratings - **Critical**: Long methods (>50 lines) in `order-processor.js` - `processOrder()` is 120 lines - **Critical**: Feature envy in `user-controller.js` - controller manipulates user model internals directly - **High**: Duplicate validation logic across `validation.js` and `order-processor.js` (5+ duplicated blocks) - **High**: Inappropriate intimacy between `legacy-user-model.js` and `user-controller.js` - **Medium**: Magic numbers scattered throughout `order-processor.js` (tax rates, thresholds) - **Medium**: Nested conditionals exceeding 3 levels in `validation.js` - **Low**: Callback-based async code in `legacy-user-model.js` (should be async/await) ### User Priorities - **Primary**: Readability and maintainability - **Secondary**: Performance (no regression) - **Pain Points**: Order processing is difficult to modify; validation logic is duplicated and inconsistent --- ## Refactoring Plan - [ ] **RF-PLAN-1.1 [Extract Method]**: - **Target**: `order-processor.js` - `processOrder()` method - **Reason**: Method exceeds 120 lines, violates single-level-of-abstraction - **Risk**: Medium - requires careful extraction to preserve order of operations - **Mitigation**: Extract in small steps, verify tests after each extraction - **Priority**: 1 - [ ] **RF-PLAN-1.2 [Introduce Strategy Pattern]**: - **Target**: `order-processor.js` - payment processing switch statement - **Reason**: Switch on payment type violates Open/Closed principle - **Risk**: Medium - requires interface definition and strategy implementations - **Mitigation**: Create strategy interface first, then migrate each case incrementally - **Priority**: 2 - [ ] **RF-PLAN-1.3 [Extract Class]**: - **Target**: `user-controller.js` - split into controller, service, and repository layers - **Reason**: Class has multiple responsibilities (HTTP handling, business logic, data access) - **Risk**: High - architectural change affecting multiple files - **Mitigation**: Extract repository first, then service layer, keeping controller thin - **Priority**: 3 - [ ] **RF-PLAN-1.4 [Replace Conditional with Polymorphism]**: - **Target**: `validation.js` - validator type dispatch - **Reason**: Long if/else chain on validator type - **Risk**: Low - well-contained change - **Mitigation**: Define validator interface, implement concrete validators - **Priority**: 4 - [ ] **RF-PLAN-1.5 [Modernization Refactoring]**: - **Target**: `legacy-user-model.js` - convert callbacks to async/await - **Reason**: Legacy callback pattern is hard to read and error-prone - **Risk**: Medium - async/await changes error handling semantics - **Mitigation**: Wrap callbacks in Promises first, then convert to async/await - **Priority**: 5 --- ## Refactoring Items ### Order Processor Refactoring - [ ] **RF-ITEM-1.1 [Extract Order Validation from processOrder()]**: - **Pattern Applied**: Extract Method - **Before**: `processOrder()` contains 40 lines of inline validation logic including order status checks, inventory verification, and customer credit checks - **After**: Extracted `validateOrder(order)` method that returns validation result object; `processOrder()` calls this method and handles the result - **Metrics**: Complexity reduced from 28 to 18; `processOrder()` reduced from 120 to 80 lines - [ ] **RF-ITEM-1.2 [Extract Pricing Calculation from processOrder()]**: - **Pattern Applied**: Extract Method with Introduce Parameter Object - **Before**: Pricing logic (discounts, tax calculation, shipping) is inline in `processOrder()` with magic numbers for tax rates (0.08, 0.10) and discount thresholds (100, 500) - **After**: Extracted `calculatePricing(order, pricingConfig)` method with a `PricingConfig` parameter object containing tax rates and discount thresholds as named constants - **Metrics**: `processOrder()` reduced to 55 lines; magic numbers eliminated - [ ] **RF-ITEM-1.3 [Replace Payment Switch with Strategy Pattern]**: - **Pattern Applied**: Strategy Pattern - **Before**: Switch statement on `order.paymentType` with cases for 'credit-card', 'paypal', 'bank-transfer', each with 15-20 lines of processing logic - **After**: `PaymentStrategy` interface with `processPayment(order)` method; concrete implementations: `CreditCardPaymentStrategy`, `PayPalPaymentStrategy`, `BankTransferPaymentStrategy`; factory method to select strategy - **Metrics**: Switch eliminated; each strategy class is under 30 lines; Open/Closed principle satisfied - [ ] **RF-ITEM-1.4 [Replace Nested Conditionals with Guard Clauses]**: - **Pattern Applied**: Guard Clauses - **Before**: Nested if statements 4 levels deep checking order status, inventory, and payment authorization - **After**: Early return guard clauses for each failure condition; main flow is linear - **Metrics**: Nesting depth reduced from 4 to 1; readability significantly improved ### Validation Module Refactoring - [ ] **RF-ITEM-2.1 [Extract Duplicate Validation Logic]**: - **Pattern Applied**: Extract Method with shared utility - **Before**: Email validation, phone validation, and postal code validation duplicated across `validation.js` and `order-processor.js` (5+ duplicated blocks of 10+ lines each) - **After**: Extracted to `src/utils/validators.js` with `validateEmail()`, `validatePhone()`, `validatePostalCode()` functions; both modules import from shared utility - **Metrics**: Duplication eliminated; validation.js reduced from 320 to 200 lines - [ ] **RF-ITEM-2.2 [Replace Conditional Dispatch with Polymorphism]**: - **Pattern Applied**: Strategy Pattern (Validator variants) - **Before**: If/else chain on `validatorType` parameter with 6 branches, each 10-15 lines - **After**: `Validator` interface with `validate(data)` method; concrete implementations: `EmailValidator`, `PhoneValidator`, `PostalCodeValidator`, `CreditCardValidator`, `DateValidator`, `RequiredFieldValidator`; registry map for type-to-validator lookup - **Metrics**: Cyclomatic complexity reduced from 19 to 8; each validator class is under 20 lines - [ ] **RF-ITEM-2.3 [Decompose Complex Boolean Expressions]**: - **Pattern Applied**: Extract Boolean Expression - **Before**: Complex condition `if (value && value.length > 0 && /^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$/.test(value) && !blacklist.includes(value))` - **After**: Named predicates: `isValidFormat(value)`, `isNotBlacklisted(value)`, `hasValidLength(value)`; combined as `if (isValidEmail(value))` - **Metrics**: Boolean expression complexity reduced; conditions are self-documenting ### User Controller Refactoring - [ ] **RF-ITEM-3.1 [Extract User Repository]**: - **Pattern Applied**: Extract Class - **Before**: `user-controller.js` contains direct database queries (SQL) inline in controller methods - **After**: Extracted `UserRepository` class with `findById()`, `findByEmail()`, `save()`, `delete()` methods; controller depends on repository interface - **Metrics**: Controller reduced from 280 to 180 lines; data access isolated - [ ] **RF-ITEM-3.2 [Extract User Service Layer]**: - **Pattern Applied**: Extract Class with Dependency Injection - **Before**: Business logic (password hashing, user validation, permission checks) is inline in controller methods - **After**: Extracted `UserService` class with `createUser()`, `updateUser()`, `deleteUser()`, `getUserProfile()` methods; controller delegates to service; service injected via constructor - **Metrics**: Controller reduced to 80 lines (HTTP handling only); service class is 150 lines with single responsibility - [ ] **RF-ITEM-3.3 [Move Method - Move User Serialization]**: - **Pattern Applied**: Move Method - **Before**: Controller has `formatUserResponse(user)` method that knows user model internals (feature envy) - **After**: Moved to `UserSerializer` class or `UserModel.toJSON()` method; controller calls `user.toJSON()` or `serializer.serialize(user)` - **Metrics**: Feature envy eliminated; controller no longer accesses user model internals directly ### Legacy User Model Modernization - [ ] **RF-ITEM-4.1 [Convert Callbacks to Promises]**: - **Pattern Applied**: Promise Wrapping - **Before**: `userModel.findById(id, function(err, user) { ... })` callback pattern throughout - **After**: `userModel.findById(id)` returns Promise; internal implementation wraps callback in `new Promise((resolve, reject) => { ... })` - **Metrics**: Callback nesting eliminated; error handling simplified - [ ] **RF-ITEM-4.2 [Convert Promises to Async/Await]**: - **Pattern Applied**: Async/Await Conversion - **Before**: Promise chains with `.then()` and `.catch()` in controller and service layers - **After**: `async` methods with `await` for asynchronous operations; try/catch for error handling - **Metrics**: Code reads linearly; error handling is more intuitive - [ ] **RF-ITEM-4.3 [Apply Optional Chaining and Nullish Coalescing]**: - **Pattern Applied**: Modern JavaScript Operators - **Before**: `user && user.profile && user.profile.address ? user.profile.address.city : 'Unknown'` and `value !== null && value !== undefined ? value : defaultValue` - **After**: `user?.profile?.address?.city ?? 'Unknown'` and `value ?? defaultValue` - **Metrics**: Null-check boilerplate reduced by 60%; code is more concise - [ ] **RF-ITEM-4.4 [Replace var with const/let and Apply Destructuring]**: - **Pattern Applied**: Variable Modernization - **Before**: `var` declarations throughout; verbose object property access `user.name`, `user.email`, `user.role` - **After**: `const` for immutable bindings, `let` for reassignment; destructuring `const { name, email, role } = user` - **Metrics**: Variable scoping is clearer; property access is more concise --- ## Proposed Code Changes ### Change 1: Extract Order Validation (RF-ITEM-1.1) ```javascript // BEFORE: order-processor.js (excerpt from processOrder()) function processOrder(order) { // ... 40 lines of validation logic if (!order.customerId) { throw new Error('Customer ID is required'); } if (order.status !== 'PENDING') { throw new Error('Order must be in PENDING status'); } if (order.items.length === 0) { throw new Error('Order must have at least one item'); } // Check inventory for (const item of order.items) { const inventory = inventoryService.get(item.productId); if (!inventory || inventory.stock < item.quantity) { throw new Error(`Insufficient inventory for product ${item.productId}`); } } // Check customer credit const customer = customerService.get(order.customerId); if (customer.creditScore < 600) { throw new Error('Customer credit score too low'); } // ... rest of processOrder } // AFTER: order-processor.js function processOrder(order) { const validationResult = validateOrder(order); if (!validationResult.isValid) { throw new Error(validationResult.error); } // ... rest of processOrder (now 80 lines) } function validateOrder(order) { if (!order.customerId) { return { isValid: false, error: 'Customer ID is required' }; } if (order.status !== 'PENDING') { return { isValid: false, error: 'Order must be in PENDING status' }; } if (order.items.length === 0) { return { isValid: false, error: 'Order must have at least one item' }; } for (const item of order.items) { const inventory = inventoryService.get(item.productId); if (!inventory || inventory.stock < item.quantity) { return { isValid: false, error: `Insufficient inventory for product ${item.productId}` }; } } const customer = customerService.get(order.customerId); if (customer.creditScore < 600) { return { isValid: false, error: 'Customer credit score too low' }; } return { isValid: true }; } ``` ### Change 2: Payment Strategy Pattern (RF-ITEM-1.3) ```javascript // BEFORE: order-processor.js (switch statement) switch (order.paymentType) { case 'credit-card': const card = paymentService.validateCard(order.paymentDetails); if (!card.isValid) throw new Error('Invalid credit card'); const charge = paymentService.charge(card.token, order.total); if (!charge.success) throw new Error('Payment failed'); break; case 'paypal': const paypalAuth = paymentService.authenticatePayPal(order.paymentDetails); if (!paypalAuth.success) throw new Error('PayPal authentication failed'); const paypalPayment = paymentService.payPalCharge(paypalAuth.token, order.total); if (!paypalPayment.success) throw new Error('PayPal payment failed'); break; case 'bank-transfer': const bankAuth = paymentService.authenticateBank(order.paymentDetails); if (!bankAuth.success) throw new Error('Bank authentication failed'); const bankPayment = paymentService.bankTransfer(bankAuth.account, order.total); if (!bankPayment.success) throw new Error('Bank transfer failed'); break; default: throw new Error(`Unsupported payment type: ${order.paymentType}`); } // AFTER: payment-strategies.js class CreditCardPaymentStrategy { processPayment(order) { const card = paymentService.validateCard(order.paymentDetails); if (!card.isValid) throw new Error('Invalid credit card'); const charge = paymentService.charge(card.token, order.total); if (!charge.success) throw new Error('Payment failed'); } } class PayPalPaymentStrategy { processPayment(order) { const paypalAuth = paymentService.authenticatePayPal(order.paymentDetails); if (!paypalAuth.success) throw new Error('PayPal authentication failed'); const paypalPayment = paymentService.payPalCharge(paypalAuth.token, order.total); if (!paypalPayment.success) throw new Error('PayPal payment failed'); } } class BankTransferPaymentStrategy { processPayment(order) { const bankAuth = paymentService.authenticateBank(order.paymentDetails); if (!bankAuth.success) throw new Error('Bank authentication failed'); const bankPayment = paymentService.bankTransfer(bankAuth.account, order.total); if (!bankPayment.success) throw new Error('Bank transfer failed'); } } // payment-strategy-factory.js function createPaymentStrategy(paymentType) { const strategies = { 'credit-card': new CreditCardPaymentStrategy(), 'paypal': new PayPalPaymentStrategy(), 'bank-transfer': new BankTransferPaymentStrategy(), }; const strategy = strategies[paymentType]; if (!strategy) { throw new Error(`Unsupported payment type: ${paymentType}`); } return strategy; } // order-processor.js (usage) const paymentStrategy = createPaymentStrategy(order.paymentType); paymentStrategy.processPayment(order); ``` ### Change 3: User Repository Extraction (RF-ITEM-3.1) ```javascript // BEFORE: user-controller.js (inline SQL) class UserController { getUser(req, res) { const userId = req.params.id; const query = 'SELECT * FROM users WHERE id = ?'; db.query(query, [userId], (err, results) => { if (err) { res.status(500).json({ error: 'Database error' }); return; } if (results.length === 0) { res.status(404).json({ error: 'User not found' }); return; } const user = results[0]; res.json({ id: user.id, name: user.name, email: user.email, role: user.role, }); }); } } // AFTER: user-repository.js class UserRepository { constructor(db) { this.db = db; } async findById(id) { const query = 'SELECT * FROM users WHERE id = ?'; const results = await this.db.query(query, [id]); return results[0] || null; } } // user-controller.js (after extraction) class UserController { constructor(userRepository, userService) { this.userRepository = userRepository; this.userService = userService; } async getUser(req, res) { try { const userId = req.params.id; const user = await this.userRepository.findById(userId); if (!user) { res.status(404).json({ error: 'User not found' }); return; } res.json(this.userService.serializeUser(user)); } catch (err) { res.status(500).json({ error: 'Database error' }); } } } ``` ### Change 4: Async/Await Conversion (RF-ITEM-4.1, RF-ITEM-4.2) ```javascript // BEFORE: legacy-user-model.js (callback pattern) UserModel.findById = function(id, callback) { db.query('SELECT * FROM users WHERE id = ?', [id], function(err, results) { if (err) { callback(err, null); return; } if (results.length === 0) { callback(null, null); return; } const user = results[0]; UserModel.getProfile(user.id, function(profileErr, profile) { if (profileErr) { callback(profileErr, null); return; } user.profile = profile; callback(null, user); }); }); }; // AFTER: legacy-user-model.js (async/await pattern) UserModel.findById = async function(id) { const results = await db.query('SELECT * FROM users WHERE id = ?', [id]); if (results.length === 0) { return null; } const user = results[0]; user.profile = await UserModel.getProfile(user.id); return user; }; UserModel.getProfile = async function(userId) { const results = await db.query('SELECT * FROM profiles WHERE user_id = ?', [userId]); return results[0] || null; }; ``` ### Change 5: Optional Chaining and Nullish Coalescing (RF-ITEM-4.3) ```javascript // BEFORE: legacy-user-model.js function getCity(user) { if (user && user.profile && user.profile.address) { return user.profile.address.city; } return 'Unknown'; } function getDisplayName(user) { return user.displayName !== null && user.displayName !== undefined ? user.displayName : user.username; } // AFTER: legacy-user-model.js function getCity(user) { return user?.profile?.address?.city ?? 'Unknown'; } function getDisplayName(user) { return user.displayName ?? user.username; } ``` --- ## Commands ### Local Commands ```bash # Run tests before refactoring to establish baseline npm test # Run tests after each refactoring step npm test # Check cyclomatic complexity npx complexity-report src/services/order-processor.js # Run linter npm run lint # Run type checking (if TypeScript) npm run typecheck # Run benchmarks (if applicable) npm run benchmark ``` ### CI Commands ```bash # Full CI pipeline npm ci npm run lint npm run test npm run build ``` --- ## Quality Assurance Task Checklist - [ ] All existing tests pass without modification to test assertions - [ ] Each refactoring step is independently verifiable and reversible - [ ] Before/after metrics demonstrate measurable improvement - [ ] No behavior changes were mixed with structural refactoring - [ ] SOLID principles are applied consistently across refactored code - [ ] Technical debt is tracked with TODO comments and severity ratings - [ ] Follow-up refactorings are documented for future iterations --- ## Follow-Up Refactorings (Future Iterations) - [ ] **RF-FUTURE-1**: Extract `OrderService` class to coordinate order processing, payment, and notification - [ ] **RF-FUTURE-2**: Introduce event-driven architecture for order state changes (Observer pattern) - [ ] **RF-FUTURE-3**: Apply Decorator pattern for cross-cutting concerns (logging, caching, metrics) - [ ] **RF-FUTURE-4**: Migrate validation to a schema-based validation library (e.g., Joi, Zod) - [ ] **RF-FUTURE-5**: Implement repository pattern for all data access layers --- ## Remaining Technical Debt | Debt Item | Severity | Estimated Effort | TODO Reference | |-----------|----------|------------------|----------------| | `order-processor.js` still has 55 lines in `processOrder()` | Medium | 2 hours | `// TODO: RF-FUTURE-1: Extract OrderService class` | | `user-controller.js` still has 80 lines (HTTP handling + error mapping) | Low | 1 hour | `// TODO: RF-FUTURE-2: Extract error handling middleware` | | `validation.js` still has 200 lines with some duplicated regex patterns | Low | 30 minutes | `// TODO: RF-FUTURE-4: Migrate to schema-based validation` | | `legacy-user-model.js` still uses `var` in 3 places | Low | 15 minutes | `// TODO: RF-ITEM-4.4: Replace remaining var with const/let` |