DO NOT MERGE feat: Redesign with Cheap Design principle in mind

CHEAP_DESIGN_OPPORTUNITIES.md

@@ -0,0 +1,217 @@
+# Cheap Design Refactoring Opportunities
+
+## Refactoring Complete ✅
+
+This document originally captured Cheap Design refactoring opportunities in the Hubot codebase. All identified opportunities have been successfully implemented through 9 incremental refactors.
+
+## Completed Refactors
+
+1. ✅ **Script loaders** (Robot.mjs): 3 duplicate methods → unified `_loadScript`
+2. ✅ **Adapter loading** (Robot.mjs): Nested if/else → resolver table strategy
+3. ✅ **Catch-all fallback** (Robot.mjs): Recursive `receive()` → in-place message conversion
+4. ✅ **Help parsing** (HelpParser.mjs): Isolated utility module
+5. ✅ **Middleware consolidation** (Middleware.mjs): Added `executeAndAllow()` helper
+6. ✅ **Response method consolidation** (Response.mjs): 6 duplicate methods → registry-driven generation
+7. ✅ **Brain user lookup** (Brain.mjs): 3 similar search methods → unified `_findUsers()` helper with predicates
+8. ✅ **Adapter method consolidation** (Adapter.mjs): Duplicate emote/reply → formatter-based delegation
+9. ✅ **Listener registration** (Robot.mjs): Duplicate message-type listeners → `LISTENER_REGISTRY` and `_registerListener()` helper
+
+---
+
+## Key Principles Applied
+
+All implementations follow Pfeifer's Cheap Design:
+- **Move variation into data**: Method registries, predicate functions, transformer tables
+- **Reduce control branching**: Replace if/else chains with table lookups and strategy patterns
+- **Separate concerns**: Extract helpers, utilities, and formatters into focused modules
+- **Leverage existing platform**: Use Object.entries(), Array.filter(), Map/Object lookups
+- **Maintain backward compatibility**: All public APIs preserved; internal refactors only
+
+## See README.md
+
+For detailed information about each completed refactor, see the "Cheap Design Principle" section in README.md.
+
+
+### 1. Response Method Consolidation ✅ (COMPLETED)
+**Location:** `src/Response.mjs` (lines 23-75)
+
+**Current Pattern:**
+Six nearly-identical methods that differ only in method name and options:
+- `send()` → calls `#runWithMiddleware('send', { plaintext: true }, ...strings)`
+- `emote()` → calls `#runWithMiddleware('emote', { plaintext: true }, ...strings)`
+- `reply()` → calls `#runWithMiddleware('reply', { plaintext: true }, ...strings)`
+- `topic()` → calls `#runWithMiddleware('topic', { plaintext: true }, ...strings)`
+- `play()` → calls `#runWithMiddleware('play', {}, ...strings)`
+- `locked()` → calls `#runWithMiddleware('locked', { plaintext: true }, ...strings)`
+
+**Cheap Design Solution:**
+Create a response method registry/table mapping method names to options:
+```javascript
+const RESPONSE_METHODS = {
+  send: { plaintext: true },
+  emote: { plaintext: true },
+  reply: { plaintext: true },
+  topic: { plaintext: true },
+  play: {},
+  locked: { plaintext: true }
+}
+
+// Single factory that generates methods
+Object.entries(RESPONSE_METHODS).forEach(([methodName, opts]) => {
+  Response.prototype[methodName] = function (...strings) {
+    return this.#runWithMiddleware(methodName, opts, ...strings)
+  }
+})
+```
+
+**Impact:** Removes ~60 lines of duplication; makes adding new response types trivial; clarifies what makes each method unique (just options).
+
+**Feasibility:** HIGH - No external API changes, fully backward compatible.
+
+---
+
+### 2. **Adapter Method Consolidation** (MEDIUM IMPACT)
+**Location:** `src/adapters/Shell.mjs` and `src/adapters/Campfire.mjs`
+
+**Current Pattern:**
+Adapters repeat similar logic across `send()`, `emote()`, `reply()`, etc. Each adapter reimplements the pattern:
+- Shell: Maps emote to `* ${str}`, reply to `${name}: ${str}`
+- Campfire: Same mapping pattern with different transport
+
+**Cheap Design Solution:**
+Create a base adapter method map that child adapters can override:
+```javascript
+class Adapter {
+  // Method transformations (data)
+  #methodTransforms = {
+    emote: (str) => `*${str}*`,
+    reply: (str, envelope) => `${envelope.user.name}: ${str}`
+  }
+
+  // Generic send implementation that child adapters extend
+  async _sendWithTransform(methodName, envelope, ...strings) {
+    const transform = this.#methodTransforms[methodName]
+    const transformed = transform ? strings.map(s => transform(s, envelope)) : strings
+    return await this.send(envelope, ...transformed)
+  }
+}
+```
+
+**Impact:** Reduces duplication across adapter implementations; standardizes method mapping.
+
+**Feasibility:** MEDIUM - Requires careful inheritance design to avoid breaking adapters.
+
+---
+
+### 3. **Brain User Lookup Consolidation** (MEDIUM IMPACT)
+**Location:** `src/Brain.mjs` (lines 160-200)
+
+**Current Pattern:**
+Multiple user lookup methods with similar filtering/search logic:
+- `userForName(name)` - exact match on name
+- `usersForRawFuzzyName(fuzzyName)` - prefix match
+- `usersForFuzzyName(fuzzyName)` - exact fallback to prefix match
+- Manual loops with similar predicates
+
+**Cheap Design Solution:**
+Create a unified user search function with pluggable predicates:
+```javascript
+_findUsers(predicate) {
+  const users = this.data.users || {}
+  return Object.values(users).filter(user => predicate(user))
+}
+
+userForName(name) {
+  const lowerName = name.toLowerCase()
+  const results = this._findUsers(u => u.name?.toLowerCase() === lowerName)
+  return results[0] ?? null
+}
+
+usersForRawFuzzyName(fuzzyName) {
+  const lowerName = fuzzyName.toLowerCase()
+  return this._findUsers(u => 
+    u.name?.toLowerCase().startsWith(lowerName)
+  )
+}
+
+usersForFuzzyName(fuzzyName) {
+  const exact = this._findUsers(u => 
+    u.name?.toLowerCase() === fuzzyName.toLowerCase()
+  )
+  if (exact.length > 0) return exact
+  return this.usersForRawFuzzyName(fuzzyName)
+}
+```
+
+**Impact:** Removes loop duplication; clarifies search logic; makes predicates reusable.
+
+**Feasibility:** HIGH - Internal refactor, no API changes.
+
+---
+
+### 4. **Router Method Registration** (LOW-MEDIUM IMPACT)
+**Location:** `src/Robot.mjs` - HTTP router methods
+
+**Current Pattern:**
+Public API methods like `hear()`, `respond()`, `enter()`, `leave()`, `topic()` all:
+1. Accept similar parameter patterns (regex/matcher, options, callback)
+2. Normalize optional parameters
+3. Create appropriate listener type
+4. Push to `this.listeners`
+
+**Cheap Design Solution:**
+Create a listener registration registry:
+```javascript
+const LISTENER_TYPES = {
+  hear: TextListener,
+  respond: (regex, options, callback) => {
+    const pattern = this.respondPattern(regex)
+    return new TextListener(this, pattern, options, callback)
+  },
+  enter: { matcher: msg => msg instanceof Message.EnterMessage },
+  leave: { matcher: msg => msg instanceof Message.LeaveMessage },
+  topic: { matcher: msg => msg instanceof Message.TopicMessage }
+}
+
+#registerListener(type, arg1, arg2, arg3) {
+  // Normalize args
+  const [matcher, options, callback] = this._normalizeListenerArgs(arg1, arg2, arg3)
+  const config = LISTENER_TYPES[type]
+  const listener = new (config.listenerClass || Listener)(this, matcher, options, callback)
+  this.listeners.push(listener)
+}
+```
+
+**Impact:** Reduces method duplication; makes pattern clearer; easier to add new listener types.
+
+**Feasibility:** MEDIUM - Requires refactoring multiple public API methods.
+
+---
+
+## Recommended Next Steps
+
+1. **Start with Response Method Consolidation** (HIGH priority, quickest win)
+   - Highest impact-to-complexity ratio
+   - Fully isolated in Response.mjs
+   - 60 lines of clear duplication
+   - Fully backward compatible
+
+2. **Then Brain User Lookup** (HIGH priority, good impact)
+   - Clean internal refactor
+   - Clarifies search predicates
+   - No API changes needed
+
+3. **Optional: Adapter Method Consolidation** (if time permits)
+   - Requires coordination across adapter files
+   - More complex inheritance changes
+   - Good long-term architectural improvement
+
+---
+
+## Refactoring Principles Applied
+
+All opportunities follow Pfeifer's Cheap Design:
+- **Move variation into data** (method name maps, predicate functions, type registries)
+- **Reduce control branching** (if/else → table lookup)
+- **Separate concerns** (each method does one thing, not n similar things)
+- **Leverage existing platform** (Object.entries, Array.filter, Map/Object lookups)

README.md

@@ -20,6 +20,70 @@ for details on getting up and running with your very own robot friend.
 In most cases, you'll probably never have to hack on this repo directly if you
 are building your own bot. But if you do, check out [CONTRIBUTING.md](CONTRIBUTING.md)
 
+## Cheap Design Principle
+
+This codebase is being incrementally refactored using Pfeifer's Cheap Design idea: simplify control by leveraging structure and existing platform capabilities. Rather than maintaining multiple near-duplicate code paths, we consolidate them and let data (like file extensions or loader state) drive behavior.
+
+### Script Loading (Applied)
+Replaced three largely similar loaders (`loadjs`, `loadmjs`, `loadts`) in `src/Robot.mjs` with a single internal `_loadScript` function invoked by thin wrappers:
+- Reduced branching: one import path, fewer error surface areas
+- Clearer warnings: unified message when default export is not a function
+- Easier extension: new script types map to the same loader
+
+### Adapter Loading (Applied)
+Eliminated nested if/else branching in `loadAdapter` by introducing a **resolver table** in `_selectAdapterResolver`. Each adapter loading strategy (built-in, local file, npm package) is now a simple, isolated resolver function:
+- **Data-driven dispatch**: Resolver selection based on adapter state and path extension, not control flow
+- **Separated concerns**: Each resolver (`_loadBuiltinAdapter`, `_loadLocalAdapter`, `_loadNpmAdapter`) has one responsibility
+- **Extensible pattern**: Adding a new adapter type requires only a new resolver and condition, no new if/else chains
+
+### Catch-All Listener Fallback (Applied)
+Eliminated recursive fallback logic in `processListeners` by replacing it with simple in-place message conversion:
+- **No recursion**: Instead of calling `receive()` recursively, convert the original message to `CatchAllMessage` and re-process the same listener loop
+- **Uniform dispatch**: Catch-all listeners now compete equally with all listeners—they just match only on `CatchAllMessage` types
+- **Cleaner control flow**: The fallback is now just data transformation, not special-case control logic
+
+### Help Parsing Extraction (Applied)
+Extracted help documentation parsing logic from `Robot.mjs` into a dedicated `src/HelpParser.mjs` utility module:
+- **Single responsibility**: `HelpParser` handles all comment extraction and section parsing; `Robot` just collects commands
+- **Reduced coupling**: Robot no longer needs to know about comment markers, header blocks, or legacy syntax detection
+- **Testable in isolation**: Help parsing logic can be tested independently without creating a Robot instance
+- **Removed 4 helper functions**: `toHeaderCommentBlock`, `isCommentLine`, `removeCommentPrefix`, and parsing logic are now in the utility
+
+### Middleware Consolidation (Applied)
+Added `executeAndAllow()` helper method to `Middleware` class to standardize the common "execute and check for false" pattern:
+- **Reduced boilerplate**: `Robot.receive()` and `Response.#runWithMiddleware()` now use `executeAndAllow()` instead of `execute()` + conditional check
+- **Clearer intent**: The method name expresses the actual operation: "execute middleware and allow if it doesn't forbid"
+- **Consistent semantics**: All middleware checks now use the same idiomatic pattern across Robot, Response, and Listener
+
+### Response Method Consolidation (Applied)
+Eliminated six nearly-identical response methods (`send`, `emote`, `reply`, `topic`, `play`, `locked`) from `src/Response.mjs`:
+- **Method registry**: All methods now defined in `RESPONSE_METHODS` object mapping method names to their options (e.g., `plaintext: true`)
+- **Dynamic generation**: Methods are generated once at module load via `Object.entries()` and prototype assignment—no code duplication
+- **Easy extension**: Adding a new response type requires only adding an entry to the registry, not writing a full method
+- **Reduced lines**: Eliminated 60+ lines of boilerplate; core logic is now just the `_runWithMiddleware` helper
+
+### Brain User Lookup Consolidation (Applied)
+Unified three similar user search methods in `src/Brain.mjs` using a single internal `_findUsers()` helper with pluggable predicates:
+- **Unified search logic**: `userForName`, `usersForRawFuzzyName`, and `usersForFuzzyName` all call `_findUsers` with different predicates
+- **Pluggable predicates**: Each predicate function encapsulates its specific matching logic (exact, prefix, etc.) independently
+- **Single return pattern**: Helper manages both single-result and array-result cases via `returnSingle` parameter
+- **Eliminated duplication**: Removed manual loops, reducing code size while keeping all public APIs identical
+
+### Adapter Method Consolidation (Applied)
+Eliminated near-duplicate `emote()` and `reply()` methods from Shell and Campfire adapters by introducing `_createSendDelegate()` in the base Adapter class:
+- **Formatter-based consolidation**: Each adapter defines formatters (e.g., `#emoteFormatter`, `#replyFormatter`) that transform strings before sending
+- **Reusable helper**: `_createSendDelegate(formatter)` returns an async function that applies the formatter to all strings, then calls `send()`
+- **Reduced adapter code**: Shell and Campfire now use one-liners: `emote = this._createSendDelegate(this.#emoteFormatter)`
+- **Extensible pattern**: New adapters can reuse the helper for any string-transformation-then-send pattern
+
+### Listener Registration Consolidation (Applied)
+Eliminated near-duplicate message-type listener registration methods (`enter`, `leave`, `topic`) in `src/Robot.mjs` using a static registry and helper:
+- **Registry-driven dispatch**: `LISTENER_REGISTRY` maps message type keys (e.g., 'enter') to their predicate functions
+- **Unified registration helper**: `_registerListener(registryKey, options, callback)` looks up the predicate and delegates to `listen()`
+- **Reduced boilerplate**: Each method (`enter`, `leave`, `topic`) now a one-liner calling `_registerListener` with its registry key
+- **Extensible design**: Adding a new message-type listener requires only adding an entry to the registry, not a full new method
+- **Clear separation**: Message types and predicates live in the registry; registration logic is isolated in the helper
+
 # Create your own Hubot instance
 
 This will create a directory called `myhubot` in the current working directory.

src/Adapter.mjs

@@ -19,8 +19,20 @@ class Adapter extends EventEmitter {
   // Returns results from adapter.
   async send (envelope, ...strings) {}
 
+  // Internal: Generate a method that formats strings and delegates to send.
+  // This consolidates the common pattern of transforming strings before sending.
+  //
+  // formatter - A function(string, envelope) that transforms each string
+  // Returns an async function that calls send with formatted strings.
+  _createSendDelegate (formatter) {
+    return async (envelope, ...strings) => {
+      const formatted = strings.map(str => formatter(str, envelope))
+      return this.send(envelope, ...formatted)
+    }
+  }
+
   // Public: Raw method for sending emote data back to the chat source.
-  // Defaults as an alias for send
+  // Defaults as an alias for send. Override in adapter for custom behavior.
   //
   // envelope - A Object with message, room and user details.
   // strings  - One or more Strings for each message to send.