Skip to content

Commit b220726

Browse files
authored
Refactor/audit reduce global state (#5530)
* refactor: Audit and reduce global state in Logo subsystem This commit introduces explicit dependency injection for the Logo subsystem to reduce reliance on the global Activity facade. Included is an audit of current dependencies and unit tests demonstrating the new pattern. * refactor: Remove implicit global usage from PhraseMaker widget This refactor follows the dependency injection pattern established for the Logo subsystem, making PhraseMaker's external dependencies explicit and testable. ## Changes Made ✅ Introduced explicit dependency injection via constructor ✅ Updated instantiation site in WidgetBlocks.js to pass dependencies ✅ Replaced all global function calls with this._deps.* pattern (289 refs) ✅ Verified runtime behavior with Jest (all tests passing) ✅ Created a clean, auditable isolation boundary ## Design Decision: this._deps.* vs Local Constants This implementation uses `this._deps.*` throughout rather than extracting local constants (e.g., `const _ = this._deps._`) for the following reasons: 1. **Explicit Dependency Tracking**: Every `this._deps.*` call makes it immediately clear that we're using an injected dependency, not a global. This aids in auditing and understanding data flow. 2. **Grep-ability**: Searching for `this._deps.` instantly reveals all external dependencies used in any method, making refactoring safer. 3. **Consistency with Logo Pattern**: Matches the approach documented in GLOBAL_STATE_AUDIT.md Phase 4, establishing a uniform pattern across the codebase. 4. **Future-Proof**: If we later need to swap implementations or add instrumentation, having explicit `this._deps.*` calls makes it easier to intercept or modify behavior. 5. **No Hidden Globals**: Local constants like `const _ = this._deps._` could be mistaken for module-level imports, reducing clarity. The trade-off is more verbose code, but the architectural benefits of explicit, traceable dependencies outweigh the verbosity cost. ## Testing All 2430 Jest tests pass. No behavioral changes. Addresses #5529 (Audit and Reduce Implicit Global State Usage) * style: Apply Prettier formatting to PhraseMaker and WidgetBlocks Automated formatting fixes to comply with Prettier code style. No functional changes. * refactor: Bind dependencies locally to reduce verbosity Apply local binding pattern to Logo and PhraseMaker to improve code readability while maintaining explicit dependency injection. ## Changes Made ✅ Logo: Bind blocks, turtles, stage as local properties ✅ PhraseMaker: Bind platformColor, docById, _, wheelnav, slicePath locally ✅ Replace this._deps.* with this.* for bound dependencies (161 refs in PhraseMaker) ✅ Update GLOBAL_STATE_AUDIT.md Phase 4 to document this pattern ✅ All tests passing (2430 tests) ## Pattern Benefits 1. **Reduced Verbosity**: `this.blocks` instead of `this.deps.blocks` 2. **Maintained Auditability**: All dependencies declared in constructor 3. **Grep-able**: Easy to search for `this.blocks` to find usages 4. **No Behavior Change**: Functionally equivalent to `this.deps.*` 5. **Explicit Contracts**: Dependencies still injected, not global This addresses reviewer feedback about excessive `this._deps.*` usage while preserving the architectural benefits of dependency injection. Addresses #5529 (Audit and Reduce Implicit Global State Usage) * style: Apply Prettier formatting to GLOBAL_STATE_AUDIT.md
1 parent 371c7e8 commit b220726

File tree

6 files changed

+1208
-387
lines changed

6 files changed

+1208
-387
lines changed

Docs/GLOBAL_STATE_AUDIT.md

Lines changed: 277 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,277 @@
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: Bind Dependencies Locally for Readability
182+
183+
To reduce verbosity while maintaining explicit dependency injection, bind commonly-used dependencies as local properties in the constructor:
184+
185+
```javascript
186+
constructor(activityOrDeps) {
187+
// ... dependency injection setup ...
188+
189+
// Bind commonly-used dependencies locally for readability
190+
// This reduces verbosity while maintaining explicit dependency injection
191+
this.blocks = this.deps.blocks;
192+
this.turtles = this.deps.turtles;
193+
this.stage = this.deps.stage;
194+
}
195+
```
196+
197+
This allows using `this.blocks.doSomething()` instead of `this.deps.blocks.doSomething()` throughout the class, while still preserving:
198+
199+
- **Explicit dependency tracking**: All dependencies are declared in the constructor
200+
- **Grep-ability**: Can search for `this.blocks` to find all usages
201+
- **Auditability**: Clear what external dependencies the class uses
202+
- **No behavior change**: Functionally equivalent to `this.deps.*` access
203+
204+
**Note**: Both `this.deps.*` (maximum audit clarity) and locally bound references (improved readability) are acceptable patterns. Choose based on the specific needs of each subsystem.
205+
206+
### Phase 5: Add Tests
207+
208+
With explicit dependencies, Logo becomes testable:
209+
210+
```javascript
211+
const mockDeps = {
212+
blocks: createMockBlocks(),
213+
turtles: createMockTurtles()
214+
// ... minimal mocks
215+
};
216+
const logo = new Logo(mockDeps);
217+
```
218+
219+
## Benefits
220+
221+
1. **Explicit Contracts**: Clear interface showing what Logo needs
222+
2. **Improved Testability**: Can inject minimal mocks instead of full Activity
223+
3. **Better Documentation**: Dependencies are self-documenting
224+
4. **Reduced Coupling**: Logo depends on interfaces, not concrete Activity
225+
5. **Migration Path**: Backward compatible during transition
226+
6. **Foundation for v4**: Establishes pattern for future architecture
227+
228+
## Risks & Mitigation
229+
230+
### Risk 1: Breaking Changes
231+
232+
**Mitigation**: Use adapter pattern to support both old and new constructors
233+
234+
### Risk 2: Incomplete Refactoring
235+
236+
**Mitigation**: Keep scope limited to Logo subsystem only
237+
238+
### Risk 3: Testing Overhead
239+
240+
**Mitigation**: Create helper functions for common mock setups
241+
242+
## Success Metrics
243+
244+
- [x] All Logo dependencies explicitly documented
245+
- [x] Dependency interface created and documented
246+
- [x] Logo constructor supports explicit dependencies
247+
- [x] Backward compatibility maintained (both patterns supported)
248+
- [ ] No behavior changes (existing tests pass) - _needs verification_
249+
- [x] New unit tests for Logo with mocked dependencies
250+
- [x] Pattern documented for future subsystems
251+
252+
## Implementation Status
253+
254+
The Logo subsystem has been updated to support the `LogoDependencies` pattern. This implementation is backward-compatible with the existing `Activity` facade.
255+
256+
### Completed
257+
258+
- [x] Initial audit of global state in Logo subsystem
259+
- [x] Creation of `LogoDependencies` class
260+
- [x] Update to `Logo` constructor to support dependency injection
261+
- [x] Unit tests for `LogoDependencies` pattern
262+
263+
### Planned
264+
265+
- [ ] Gradual refactoring of internal `this.activity` references
266+
- [ ] Application of pattern to other core subsystems (Blocks, Turtles)
267+
268+
## References
269+
270+
- Issue #2767: Identify and/or fix high-level inconsistencies
271+
- Related work: PhraseMaker widget isolation
272+
- RequireJS migration considerations
273+
274+
---
275+
276+
**Status**: Phase 3 Complete
277+
**Owner**: @vanshika2720

0 commit comments

Comments
 (0)