|
| 1 | +# Global State Audit - Logo Subsystem |
| 2 | + |
| 3 | +## Purpose |
| 4 | + |
| 5 | +This document audits implicit global state dependencies in the **Logo subsystem** as part of the architectural improvement initiative to reduce global coupling and improve testability. |
| 6 | + |
| 7 | +## Scope |
| 8 | + |
| 9 | +- **Subsystem**: Logo (`js/logo.js`) |
| 10 | +- **Related Issue**: Audit and Reduce Implicit Global State Usage |
| 11 | +- **Date**: 2026-02-04 |
| 12 | + |
| 13 | +## Current Architecture |
| 14 | + |
| 15 | +### Logo Class Dependencies |
| 16 | + |
| 17 | +The Logo class currently depends on a single `activity` object passed to its constructor, which acts as a facade to access multiple subsystems: |
| 18 | + |
| 19 | +```javascript |
| 20 | +constructor(activity) { |
| 21 | + this.activity = activity; |
| 22 | + // ... initialization |
| 23 | +} |
| 24 | +``` |
| 25 | + |
| 26 | +### Identified Global Dependencies (via `this.activity`) |
| 27 | + |
| 28 | +#### 1. **Blocks Subsystem** (`this.activity.blocks`) |
| 29 | + |
| 30 | +- **Usage Count**: ~50+ references |
| 31 | +- **Access Pattern**: `this.activity.blocks.*` |
| 32 | +- **Key Methods Used**: |
| 33 | + - `blockList` - Direct property access |
| 34 | + - `unhighlightAll()` - Visual feedback |
| 35 | + - `bringToTop()` - Z-index management |
| 36 | + - `showBlocks()` / `hideBlocks()` - Visibility control |
| 37 | + - `sameGeneration()` - Block relationship queries |
| 38 | + - `visible` - State query |
| 39 | + |
| 40 | +**Impact**: High - Logo heavily depends on Blocks for execution flow and visual feedback |
| 41 | + |
| 42 | +#### 2. **Turtles Subsystem** (`this.activity.turtles`) |
| 43 | + |
| 44 | +- **Usage Count**: ~80+ references |
| 45 | +- **Access Pattern**: `this.activity.turtles.*` |
| 46 | +- **Key Methods Used**: |
| 47 | + - `turtleList` - Direct iteration over turtles |
| 48 | + - `ithTurtle(index)` - Get specific turtle |
| 49 | + - `getTurtle(index)` - Get turtle by index |
| 50 | + - `getTurtleCount()` - Count query |
| 51 | + - `add()` - Create new turtle |
| 52 | + - `markAllAsStopped()` - State management |
| 53 | + |
| 54 | +**Impact**: Critical - Logo is the execution engine for turtle commands |
| 55 | + |
| 56 | +#### 3. **Stage** (`this.activity.stage`) |
| 57 | + |
| 58 | +- **Usage Count**: ~10+ references |
| 59 | +- **Access Pattern**: `this.activity.stage.*` |
| 60 | +- **Key Methods Used**: |
| 61 | + - `addEventListener()` - Event handling |
| 62 | + - `removeEventListener()` - Cleanup |
| 63 | + |
| 64 | +**Impact**: Medium - Used for interaction and event handling |
| 65 | + |
| 66 | +#### 4. **UI/Error Handling** |
| 67 | + |
| 68 | +- **Methods**: |
| 69 | + - `this.activity.errorMsg()` - Error display |
| 70 | + - `this.activity.hideMsgs()` - Message management |
| 71 | + - `this.activity.saveLocally()` - Persistence |
| 72 | + |
| 73 | +**Impact**: Medium - UI feedback and state persistence |
| 74 | + |
| 75 | +#### 5. **Configuration/State** |
| 76 | + |
| 77 | +- **Properties**: |
| 78 | + - `this.activity.showBlocksAfterRun` - Boolean flag |
| 79 | + - `this.activity.onStopTurtle` - Callback |
| 80 | + - `this.activity.onRunTurtle` - Callback |
| 81 | + - `this.activity.meSpeak` - Speech synthesis |
| 82 | + |
| 83 | +**Impact**: Low-Medium - Configuration and callbacks |
| 84 | + |
| 85 | +## Dependency Graph |
| 86 | + |
| 87 | +``` |
| 88 | +Logo |
| 89 | + ├─► Activity (facade) |
| 90 | + ├─► Blocks (execution context, visual feedback) |
| 91 | + ├─► Turtles (command execution, state management) |
| 92 | + ├─► Stage (event handling) |
| 93 | + ├─► ErrorMsg (UI feedback) |
| 94 | + └─► Configuration (flags, callbacks) |
| 95 | +``` |
| 96 | + |
| 97 | +## Analysis |
| 98 | + |
| 99 | +### Current Problems |
| 100 | + |
| 101 | +1. **Tight Coupling**: Logo cannot be instantiated or tested without a full Activity object |
| 102 | +2. **Hidden Dependencies**: The Activity facade obscures what Logo actually needs |
| 103 | +3. **Circular Dependencies**: Logo ↔ Activity ↔ Blocks ↔ Turtles creates complex initialization |
| 104 | +4. **Testing Difficulty**: Mocking requires recreating entire Activity object graph |
| 105 | +5. **Unclear Contracts**: No explicit interface defining what Logo needs from Activity |
| 106 | + |
| 107 | +### Unavoidable Dependencies |
| 108 | + |
| 109 | +Some dependencies are fundamental to Logo's role as the execution engine: |
| 110 | + |
| 111 | +- **Turtles**: Logo executes turtle commands, so this dependency is core |
| 112 | +- **Blocks**: Logo interprets block programs, so this dependency is core |
| 113 | + |
| 114 | +### Refactorable Dependencies |
| 115 | + |
| 116 | +These could be made explicit or injected: |
| 117 | + |
| 118 | +- **Stage**: Could be injected as an event bus interface |
| 119 | +- **Error handling**: Could be injected as a logger/error handler interface |
| 120 | +- **Configuration**: Could be passed as a config object |
| 121 | +- **Callbacks**: Could be injected explicitly |
| 122 | + |
| 123 | +## Proposed Refactoring Strategy |
| 124 | + |
| 125 | +### Phase 1: Document Current State ✅ |
| 126 | + |
| 127 | +- Create this audit document |
| 128 | +- Identify all `this.activity.*` references |
| 129 | +- Categorize by subsystem and impact |
| 130 | + |
| 131 | +### Phase 2: Extract Interfaces (Small, Safe Changes) |
| 132 | + |
| 133 | +Create explicit interfaces for Logo's dependencies: |
| 134 | + |
| 135 | +```javascript |
| 136 | +// LogoDependencies interface |
| 137 | +class LogoDependencies { |
| 138 | + constructor({ |
| 139 | + blocks, // Blocks subsystem |
| 140 | + turtles, // Turtles subsystem |
| 141 | + stage, // Event bus |
| 142 | + errorHandler, // Error display |
| 143 | + messageHandler, // Message display |
| 144 | + storage, // Persistence |
| 145 | + config, // Configuration |
| 146 | + callbacks // onStop, onRun, etc. |
| 147 | + }) { |
| 148 | + this.blocks = blocks; |
| 149 | + this.turtles = turtles; |
| 150 | + this.stage = stage; |
| 151 | + this.errorHandler = errorHandler; |
| 152 | + this.messageHandler = messageHandler; |
| 153 | + this.storage = storage; |
| 154 | + this.config = config; |
| 155 | + this.callbacks = callbacks; |
| 156 | + } |
| 157 | +} |
| 158 | +``` |
| 159 | + |
| 160 | +### Phase 3: Refactor Constructor (Backward Compatible) |
| 161 | + |
| 162 | +```javascript |
| 163 | +constructor(activityOrDeps) { |
| 164 | + // Support both old and new patterns |
| 165 | + if (activityOrDeps.blocks && activityOrDeps.turtles) { |
| 166 | + // New explicit dependencies |
| 167 | + this.deps = activityOrDeps; |
| 168 | + } else { |
| 169 | + // Old activity facade - extract dependencies |
| 170 | + this.deps = new LogoDependencies({ |
| 171 | + blocks: activityOrDeps.blocks, |
| 172 | + turtles: activityOrDeps.turtles, |
| 173 | + stage: activityOrDeps.stage, |
| 174 | + errorHandler: activityOrDeps.errorMsg.bind(activityOrDeps), |
| 175 | + // ... etc |
| 176 | + }); |
| 177 | + } |
| 178 | +} |
| 179 | +``` |
| 180 | + |
| 181 | +### Phase 4: Update Internal References |
| 182 | + |
| 183 | +Replace `this.activity.*` with `this.deps.*` throughout Logo class |
| 184 | + |
| 185 | +### Phase 5: Add Tests |
| 186 | + |
| 187 | +With explicit dependencies, Logo becomes testable: |
| 188 | + |
| 189 | +```javascript |
| 190 | +const mockDeps = { |
| 191 | + blocks: createMockBlocks(), |
| 192 | + turtles: createMockTurtles() |
| 193 | + // ... minimal mocks |
| 194 | +}; |
| 195 | +const logo = new Logo(mockDeps); |
| 196 | +``` |
| 197 | + |
| 198 | +## Benefits |
| 199 | + |
| 200 | +1. **Explicit Contracts**: Clear interface showing what Logo needs |
| 201 | +2. **Improved Testability**: Can inject minimal mocks instead of full Activity |
| 202 | +3. **Better Documentation**: Dependencies are self-documenting |
| 203 | +4. **Reduced Coupling**: Logo depends on interfaces, not concrete Activity |
| 204 | +5. **Migration Path**: Backward compatible during transition |
| 205 | +6. **Foundation for v4**: Establishes pattern for future architecture |
| 206 | + |
| 207 | +## Risks & Mitigation |
| 208 | + |
| 209 | +### Risk 1: Breaking Changes |
| 210 | + |
| 211 | +**Mitigation**: Use adapter pattern to support both old and new constructors |
| 212 | + |
| 213 | +### Risk 2: Incomplete Refactoring |
| 214 | + |
| 215 | +**Mitigation**: Keep scope limited to Logo subsystem only |
| 216 | + |
| 217 | +### Risk 3: Testing Overhead |
| 218 | + |
| 219 | +**Mitigation**: Create helper functions for common mock setups |
| 220 | + |
| 221 | +## Success Metrics |
| 222 | + |
| 223 | +- [x] All Logo dependencies explicitly documented |
| 224 | +- [x] Dependency interface created and documented |
| 225 | +- [x] Logo constructor supports explicit dependencies |
| 226 | +- [x] Backward compatibility maintained (both patterns supported) |
| 227 | +- [ ] No behavior changes (existing tests pass) - _needs verification_ |
| 228 | +- [x] New unit tests for Logo with mocked dependencies |
| 229 | +- [x] Pattern documented for future subsystems |
| 230 | + |
| 231 | +## Implementation Status |
| 232 | + |
| 233 | +The Logo subsystem has been updated to support the `LogoDependencies` pattern. This implementation is backward-compatible with the existing `Activity` facade. |
| 234 | + |
| 235 | +### Completed |
| 236 | + |
| 237 | +- [x] Initial audit of global state in Logo subsystem |
| 238 | +- [x] Creation of `LogoDependencies` class |
| 239 | +- [x] Update to `Logo` constructor to support dependency injection |
| 240 | +- [x] Unit tests for `LogoDependencies` pattern |
| 241 | + |
| 242 | +### Planned |
| 243 | + |
| 244 | +- [ ] Gradual refactoring of internal `this.activity` references |
| 245 | +- [ ] Application of pattern to other core subsystems (Blocks, Turtles) |
| 246 | + |
| 247 | +## References |
| 248 | + |
| 249 | +- Issue #2767: Identify and/or fix high-level inconsistencies |
| 250 | +- Related work: PhraseMaker widget isolation |
| 251 | +- RequireJS migration considerations |
| 252 | + |
| 253 | +--- |
| 254 | + |
| 255 | +**Status**: Phase 3 Complete |
| 256 | +**Owner**: @vanshika2720 |
0 commit comments