Skip to content

Commit 1935563

Browse files
committed
docs: Add god class refactoring plan prompt
Document the architectural issues with sequencing module: - 3 files over 3000 lines each - 21 duplicated constraint validation checks - 5 methods doing overlapping choice validation - Coverage stuck at ~70% due to duplicate code paths Outlines phased refactoring strategy to extract validators, handlers, and traversal logic into focused single-purpose classes.
1 parent 2f52844 commit 1935563

File tree

1 file changed

+137
-0
lines changed

1 file changed

+137
-0
lines changed

GOD-CLASS-REFACTORING.md

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
# God Class Refactoring Plan - Sequencing Module
2+
3+
## Problem Statement
4+
5+
The SCORM 2004 sequencing implementation has several "god classes" that are too large, have too many responsibilities, and contain significant code duplication. This makes the code:
6+
7+
1. **Hard to test** - Coverage is stuck at ~70% because duplicate code paths mean tests hit one implementation but miss the copy
8+
2. **Hard to maintain** - Changes require updating multiple places
9+
3. **Hard to understand** - 3000+ line files with 60-119 methods each
10+
11+
## The God Classes
12+
13+
| File | Lines | Methods | Coverage | Problem |
14+
|------|-------|---------|----------|---------|
15+
| `overall_sequencing_process.ts` | 3,733 | 119 | 83% | Orchestrates everything, duplicates validation |
16+
| `sequencing_process.ts` | 3,036 | 63 | 70% | Duplicated constraint validation logic |
17+
| `Scorm2004API.ts` | 3,253 | 65 | 86% | API + business logic mixed |
18+
| `activity.ts` | 2,222 | ~60 | 95% | Data + behavior + validation mixed |
19+
| `rollup_process.ts` | 1,718 | 41 | 83% | Large but more focused |
20+
| `BaseAPI.ts` | 1,915 | 20 | 92% | Acceptable size for base class |
21+
22+
## Specific Issues in `sequencing_process.ts`
23+
24+
### Issue 1: Duplicated Constraint Validation (21 occurrences)
25+
26+
The same `constrainChoice`, `forwardOnly`, and `preventActivation` checks appear in multiple places:
27+
28+
```
29+
Lines 569-578: choiceSequencingRequestProcess() - constrainChoice backward check
30+
Lines 2932-2937: validateConstraintsAtAncestorLevel() - SAME CHECK, different path
31+
```
32+
33+
Tests hit lines 569-578 and pass, but lines 2932-2937 remain uncovered because they're duplicate code reached via `getAvailableChoices()`.
34+
35+
### Issue 2: Five Methods Doing the Same Thing
36+
37+
All of these validate choice constraints with slight variations:
38+
39+
- `validateChoicePathConstraints()` - 82 lines
40+
- `validateConstraintsAtAncestorLevel()` - 81 lines
41+
- `validateConstrainChoiceForFlow()` - 83 lines
42+
- `evaluateConstrainChoiceForTraversal()` - 74 lines
43+
- `checkConstrainedChoiceBoundary()` - 111 lines
44+
45+
**Total: 431 lines of overlapping validation logic**
46+
47+
### Issue 3: Methods Too Long
48+
49+
- `evaluateRuleConditions()` - 237 lines
50+
- `choiceSequencingRequestProcess()` - 200 lines
51+
- `flowTreeTraversalSubprocess()` - 129 lines
52+
- `checkConstrainedChoiceBoundary()` - 111 lines
53+
54+
## Proposed Refactoring Strategy
55+
56+
### Phase 1: Extract Constraint Validators
57+
58+
Create a single `ChoiceConstraintValidator` class that handles ALL constraint validation:
59+
60+
```typescript
61+
// New file: src/cmi/scorm2004/sequencing/validators/choice_constraint_validator.ts
62+
export class ChoiceConstraintValidator {
63+
validateConstrainChoice(ancestor: Activity, current: Activity, target: Activity): ValidationResult
64+
validateForwardOnly(ancestor: Activity, current: Activity, target: Activity): ValidationResult
65+
validatePreventActivation(ancestor: Activity, target: Activity): ValidationResult
66+
validateAllAncestorConstraints(current: Activity, target: Activity): ValidationResult
67+
}
68+
```
69+
70+
This eliminates the 5 duplicate methods and 21 scattered constraint checks.
71+
72+
### Phase 2: Extract Request Handlers
73+
74+
Break `sequencing_process.ts` into focused request handlers:
75+
76+
```
77+
src/cmi/scorm2004/sequencing/
78+
├── sequencing_process.ts # Coordinator only (~500 lines)
79+
├── handlers/
80+
│ ├── choice_request_handler.ts # All choice logic
81+
│ ├── flow_request_handler.ts # Continue/Previous
82+
│ ├── exit_request_handler.ts # Exit/ExitAll/Abandon
83+
│ └── retry_request_handler.ts # Retry/RetryAll
84+
├── validators/
85+
│ ├── choice_constraint_validator.ts
86+
│ └── navigation_validator.ts
87+
└── traversal/
88+
├── flow_traversal.ts
89+
└── choice_traversal.ts
90+
```
91+
92+
### Phase 3: Simplify `overall_sequencing_process.ts`
93+
94+
This 3,733-line file orchestrates the sequencing loop. Extract:
95+
96+
- Termination logic → `termination_handler.ts`
97+
- Delivery logic → `delivery_handler.ts`
98+
- Navigation validity → `navigation_validity_service.ts`
99+
100+
### Phase 4: Refactor Tests Around New Structure
101+
102+
Current tests exercise the god classes through high-level entry points. After refactoring:
103+
104+
1. **Unit tests** for each extracted class (validators, handlers, traversal)
105+
2. **Integration tests** for the coordinator classes
106+
3. **Remove duplicate test scenarios** that exist only because code was duplicated
107+
108+
## Success Criteria
109+
110+
1. No source file > 800 lines
111+
2. No class > 30 methods
112+
3. Zero duplicated validation logic
113+
4. Coverage > 85% on all sequencing files
114+
5. All existing tests still pass
115+
116+
## Files to Read First
117+
118+
Before planning, read these to understand the current structure:
119+
120+
1. `/src/cmi/scorm2004/sequencing/sequencing_process.ts` - Lines 440-600 (choice handling)
121+
2. `/src/cmi/scorm2004/sequencing/sequencing_process.ts` - Lines 2800-2950 (duplicate validation)
122+
3. `/src/cmi/scorm2004/sequencing/overall_sequencing_process.ts` - Lines 1-200 (main loop)
123+
4. `/test/cmi/scorm2004/sequencing/sequencing_process.spec.ts` - Understand test patterns
124+
125+
## Constraints
126+
127+
- Must maintain SCORM 2004 4th Edition compliance
128+
- Must not break existing public API (`Scorm2004API`)
129+
- Must maintain backward compatibility with existing LMS integrations
130+
- Incremental refactoring preferred (one extraction at a time, tests passing between each)
131+
132+
## Questions to Answer During Planning
133+
134+
1. Which extraction provides the most value with least risk?
135+
2. Can we refactor incrementally or does it need to be all-at-once?
136+
3. Are there hidden dependencies between the duplicate code paths?
137+
4. Should we add integration tests before refactoring as a safety net?

0 commit comments

Comments
 (0)