372 lines
8.7 KiB
Markdown
372 lines
8.7 KiB
Markdown
<!-- Powered by BMAD™ Core -->
|
|
|
|
# tdd-refactor
|
|
|
|
Safely refactor code while keeping all tests green - the "Refactor" phase of TDD.
|
|
|
|
## Purpose
|
|
|
|
Improve code quality, eliminate duplication, and enhance design while maintaining all existing functionality. This is the "Refactor" phase of TDD where we make the code clean and maintainable.
|
|
|
|
## Prerequisites
|
|
|
|
- All tests are passing (tdd.status: green)
|
|
- Implementation is complete and functional
|
|
- Test suite provides safety net for refactoring
|
|
- Code follows basic project standards
|
|
|
|
## Inputs
|
|
|
|
```yaml
|
|
required:
|
|
- story_id: '{epic}.{story}' # e.g., "1.3"
|
|
- story_path: '{devStoryLocation}/{epic}.{story}.*.md' # Path from core-config.yaml
|
|
- passing_tests: # All tests should be green
|
|
- id: test identifier
|
|
- status: passing
|
|
- implementation_files: # Source files to potentially refactor
|
|
- path: file path
|
|
- purpose: what it does
|
|
```
|
|
|
|
## Process
|
|
|
|
### 1. Identify Refactoring Opportunities
|
|
|
|
**Code Smells to Look For:**
|
|
|
|
```yaml
|
|
common_smells:
|
|
duplication:
|
|
- Repeated code blocks
|
|
- Similar logic in different places
|
|
- Copy-paste patterns
|
|
|
|
complexity:
|
|
- Long methods/functions (>10-15 lines)
|
|
- Too many parameters (>3-4)
|
|
- Nested conditions (>2-3 levels)
|
|
- Complex boolean expressions
|
|
|
|
naming:
|
|
- Unclear variable names
|
|
- Non-descriptive function names
|
|
- Inconsistent naming conventions
|
|
|
|
structure:
|
|
- God objects/classes doing too much
|
|
- Primitive obsession
|
|
- Feature envy (method using more from other class)
|
|
- Long parameter lists
|
|
```
|
|
|
|
### 2. Plan Refactoring Steps
|
|
|
|
**Refactoring Strategy:**
|
|
|
|
- **One change at a time:** Make small, atomic improvements
|
|
- **Run tests after each change:** Ensure no functionality breaks
|
|
- **Commit frequently:** Create checkpoints for easy rollback
|
|
- **Improve design:** Move toward better architecture
|
|
|
|
**Common Refactoring Techniques:**
|
|
|
|
```yaml
|
|
extract_methods:
|
|
when: 'Function is too long or doing multiple things'
|
|
technique: 'Extract complex logic into named methods'
|
|
|
|
rename_variables:
|
|
when: "Names don't clearly express intent"
|
|
technique: 'Use intention-revealing names'
|
|
|
|
eliminate_duplication:
|
|
when: 'Same code appears in multiple places'
|
|
technique: 'Extract to shared function/method'
|
|
|
|
simplify_conditionals:
|
|
when: 'Complex boolean logic is hard to understand'
|
|
technique: 'Extract to well-named boolean methods'
|
|
|
|
introduce_constants:
|
|
when: 'Magic numbers or strings appear repeatedly'
|
|
technique: 'Create named constants'
|
|
```
|
|
|
|
### 3. Execute Refactoring
|
|
|
|
**Step-by-Step Process:**
|
|
|
|
1. **Choose smallest improvement**
|
|
2. **Make the change**
|
|
3. **Run all tests**
|
|
4. **Commit if green**
|
|
5. **Repeat**
|
|
|
|
**Example Refactoring Sequence:**
|
|
|
|
```javascript
|
|
// Before refactoring
|
|
function createUser(data) {
|
|
if (!data.email.includes('@') || data.email.length < 5) {
|
|
throw new Error('Invalid email format');
|
|
}
|
|
if (!data.name || data.name.trim().length === 0) {
|
|
throw new Error('Name is required');
|
|
}
|
|
return {
|
|
id: Math.floor(Math.random() * 1000000),
|
|
...data,
|
|
createdAt: new Date().toISOString(),
|
|
};
|
|
}
|
|
|
|
// After refactoring - Step 1: Extract validation
|
|
function validateEmail(email) {
|
|
return email.includes('@') && email.length >= 5;
|
|
}
|
|
|
|
function validateName(name) {
|
|
return name && name.trim().length > 0;
|
|
}
|
|
|
|
function createUser(data) {
|
|
if (!validateEmail(data.email)) {
|
|
throw new Error('Invalid email format');
|
|
}
|
|
if (!validateName(data.name)) {
|
|
throw new Error('Name is required');
|
|
}
|
|
return {
|
|
id: Math.floor(Math.random() * 1000000),
|
|
...data,
|
|
createdAt: new Date().toISOString(),
|
|
};
|
|
}
|
|
|
|
// After refactoring - Step 2: Extract ID generation
|
|
function generateUserId() {
|
|
return Math.floor(Math.random() * 1000000);
|
|
}
|
|
|
|
function createUser(data) {
|
|
if (!validateEmail(data.email)) {
|
|
throw new Error('Invalid email format');
|
|
}
|
|
if (!validateName(data.name)) {
|
|
throw new Error('Name is required');
|
|
}
|
|
return {
|
|
id: generateUserId(),
|
|
...data,
|
|
createdAt: new Date().toISOString(),
|
|
};
|
|
}
|
|
```
|
|
|
|
### 4. Test After Each Change
|
|
|
|
**Critical Rule:** Never proceed without green tests
|
|
|
|
```bash
|
|
# Run tests after each refactoring step
|
|
npm test
|
|
pytest
|
|
go test ./...
|
|
|
|
# If tests fail:
|
|
# 1. Undo the change
|
|
# 2. Understand what broke
|
|
# 3. Try smaller refactoring
|
|
# 4. Fix tests if they need updating (rare)
|
|
```
|
|
|
|
### 5. Collaborate with QA Agent
|
|
|
|
**When to involve QA:**
|
|
|
|
- Tests need updating due to interface changes
|
|
- New test cases identified during refactoring
|
|
- Questions about test coverage adequacy
|
|
- Validation of refactoring safety
|
|
|
|
### 6. Update Story Documentation
|
|
|
|
Track refactoring progress:
|
|
|
|
```yaml
|
|
tdd:
|
|
status: refactor # or done if complete
|
|
cycle: 1
|
|
refactoring_notes:
|
|
- extracted_methods: ['validateEmail', 'validateName', 'generateUserId']
|
|
- eliminated_duplication: 'Email validation logic'
|
|
- improved_readability: 'Function names now express intent'
|
|
```
|
|
|
|
## Output Requirements
|
|
|
|
### 1. Improved Code Quality
|
|
|
|
**Measurable Improvements:**
|
|
|
|
- Reduced code duplication
|
|
- Clearer naming and structure
|
|
- Smaller, focused functions
|
|
- Better separation of concerns
|
|
|
|
### 2. Maintained Test Coverage
|
|
|
|
```bash
|
|
# All tests still passing
|
|
✅ UserService > should create user with valid email
|
|
✅ UserService > should reject user with invalid email
|
|
✅ UserService > should require valid name
|
|
|
|
3 passing, 0 failing
|
|
```
|
|
|
|
### 3. Story File Updates
|
|
|
|
Append to TDD section:
|
|
|
|
```markdown
|
|
## TDD Progress
|
|
|
|
### Refactor Phase - Cycle 1
|
|
|
|
**Date:** {current_date}
|
|
**Agents:** James (Dev) & Quinn (QA)
|
|
|
|
**Refactoring Completed:**
|
|
|
|
- ✅ Extracted validation functions for better readability
|
|
- ✅ Eliminated duplicate email validation logic
|
|
- ✅ Introduced generateUserId() for testability
|
|
- ✅ Simplified createUser() main logic
|
|
|
|
**Code Quality Improvements:**
|
|
|
|
- Function length reduced from 12 to 6 lines
|
|
- Three reusable validation functions created
|
|
- Magic numbers eliminated
|
|
- Test coverage maintained at 100%
|
|
|
|
**Files Modified:**
|
|
|
|
- src/services/user-service.js (refactored)
|
|
|
|
**All Tests Passing:** ✅
|
|
|
|
**Next Step:** Story ready for review or next TDD cycle
|
|
```
|
|
|
|
## Refactoring Guidelines
|
|
|
|
### Safe Refactoring Practices
|
|
|
|
**Always Safe:**
|
|
|
|
- Rename variables/functions
|
|
- Extract methods
|
|
- Inline temporary variables
|
|
- Replace magic numbers with constants
|
|
|
|
**Potentially Risky:**
|
|
|
|
- Changing method signatures
|
|
- Modifying class hierarchies
|
|
- Altering error handling
|
|
- Changing async/sync behavior
|
|
|
|
**Never Do During Refactor:**
|
|
|
|
- Add new features
|
|
- Change external behavior
|
|
- Remove existing functionality
|
|
- Skip running tests
|
|
|
|
### Code Quality Metrics
|
|
|
|
**Before/After Comparison:**
|
|
|
|
```yaml
|
|
metrics_to_track:
|
|
cyclomatic_complexity: 'Lower is better'
|
|
function_length: 'Shorter is generally better'
|
|
duplication_percentage: 'Should decrease'
|
|
test_coverage: 'Should maintain 100%'
|
|
|
|
acceptable_ranges:
|
|
function_length: '5-15 lines for most functions'
|
|
parameters: '0-4 parameters per function'
|
|
nesting_depth: 'Maximum 3 levels'
|
|
```
|
|
|
|
## Advanced Refactoring Techniques
|
|
|
|
### Design Pattern Introduction
|
|
|
|
**When appropriate:**
|
|
|
|
- Template Method for algorithmic variations
|
|
- Strategy Pattern for behavior selection
|
|
- Factory Pattern for object creation
|
|
- Observer Pattern for event handling
|
|
|
|
**Caution:** Only introduce patterns if they simplify the code
|
|
|
|
### Architecture Improvements
|
|
|
|
```yaml
|
|
layering:
|
|
- Separate business logic from presentation
|
|
- Extract data access concerns
|
|
- Isolate external dependencies
|
|
|
|
dependency_injection:
|
|
- Make dependencies explicit
|
|
- Enable easier testing
|
|
- Improve modularity
|
|
|
|
error_handling:
|
|
- Consistent error types
|
|
- Meaningful error messages
|
|
- Proper error propagation
|
|
```
|
|
|
|
## Error Handling
|
|
|
|
**If tests fail during refactoring:**
|
|
|
|
1. **Undo immediately** - Use git to revert
|
|
2. **Analyze the failure** - What assumption was wrong?
|
|
3. **Try smaller steps** - More atomic refactoring
|
|
4. **Consider test updates** - Only if interface must change
|
|
|
|
**If code becomes more complex:**
|
|
|
|
- Refactoring went wrong direction
|
|
- Revert and try different approach
|
|
- Consider if change is actually needed
|
|
|
|
## Completion Criteria
|
|
|
|
- [ ] All identified code smells addressed or documented
|
|
- [ ] All tests remain green throughout process
|
|
- [ ] Code is more readable and maintainable
|
|
- [ ] No new functionality added during refactoring
|
|
- [ ] Story TDD status updated appropriately
|
|
- [ ] Refactoring changes committed with clear messages
|
|
- [ ] Code quality metrics improved or maintained
|
|
- [ ] Ready for story completion or next TDD cycle
|
|
|
|
## Key Principles
|
|
|
|
- **Green Bar:** Never proceed with failing tests
|
|
- **Small Steps:** Make incremental improvements
|
|
- **Behavior Preservation:** External behavior must remain identical
|
|
- **Frequent Commits:** Create rollback points
|
|
- **Test First:** Let tests guide refactoring safety
|
|
- **Collaborative:** Work with QA when test updates needed
|