diff --git a/CHEAP_DESIGN_OPPORTUNITIES.md b/CHEAP_DESIGN_OPPORTUNITIES.md new file mode 100644 index 000000000..55c093b66 --- /dev/null +++ b/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) diff --git a/README.md b/README.md index b10b825c2..4e2086c6b 100644 --- a/README.md +++ b/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. diff --git a/src/Adapter.mjs b/src/Adapter.mjs index 45fd15d82..3bc4901eb 100644 --- a/src/Adapter.mjs +++ b/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. diff --git a/src/Brain.mjs b/src/Brain.mjs index 5e17d0f2e..68f64fa48 100644 --- a/src/Brain.mjs +++ b/src/Brain.mjs @@ -188,21 +188,43 @@ class Brain extends EventEmitter { return user } - // Public: Get a User object given a name. + // Internal: Find users matching a predicate function. // - // Returns a User instance for the user with the specified name. - userForName (name) { + // predicate - A function(user, searchTerm) that returns true if user matches. + // searchTerm - The value to search for (name, fuzzy name, etc). + // returnSingle - If true, return first match; if false, return array. + // + // Returns a User instance (if returnSingle) or array of User instances. + _findUsers (predicate, searchTerm, returnSingle = false) { + const users = this.data.users || {} let result = null - const lowerName = name.toLowerCase() - - for (const k in this.data.users || {}) { - const userName = this.data.users[k].name - if (userName != null && userName.toString().toLowerCase() === lowerName) { - result = this.data.users[k] + const matches = [] + + for (const k in users) { + if (predicate(users[k], searchTerm)) { + if (returnSingle) { + result = users[k] + break + } + matches.push(users[k]) } } - return result + return returnSingle ? result : matches + } + + // Public: Get a User object given a name. + // + // Returns a User instance for the user with the specified name. + userForName (name) { + return this._findUsers( + (user, searchTerm) => { + const userName = user.name + return userName != null && userName.toString().toLowerCase() === searchTerm.toLowerCase() + }, + name, + true + ) } // Public: Get all users whose names match fuzzyName. Currently, match @@ -211,17 +233,11 @@ class Brain extends EventEmitter { // // Returns an Array of User instances matching the fuzzy name. usersForRawFuzzyName (fuzzyName) { - const lowerFuzzyName = fuzzyName.toLowerCase() - - const users = this.data.users || {} - - return Object.keys(users).reduce((result, key) => { - const user = users[key] - if (user.name.toLowerCase().lastIndexOf(lowerFuzzyName, 0) === 0) { - result.push(user) - } - return result - }, []) + return this._findUsers( + (user, searchTerm) => user.name.toLowerCase().lastIndexOf(searchTerm.toLowerCase(), 0) === 0, + fuzzyName, + false + ) } // Public: If fuzzyName is an exact match for a user, returns an array with @@ -231,10 +247,9 @@ class Brain extends EventEmitter { // Returns an Array of User instances matching the fuzzy name. usersForFuzzyName (fuzzyName) { const matchedUsers = this.usersForRawFuzzyName(fuzzyName) - const lowerFuzzyName = fuzzyName.toLowerCase() - const fuzzyMatchedUsers = matchedUsers.filter(user => user.name.toLowerCase() === lowerFuzzyName) + const exactMatches = matchedUsers.filter(user => user.name.toLowerCase() === fuzzyName.toLowerCase()) - return fuzzyMatchedUsers.length > 0 ? fuzzyMatchedUsers : matchedUsers + return exactMatches.length > 0 ? exactMatches : matchedUsers } } diff --git a/src/HelpParser.mjs b/src/HelpParser.mjs new file mode 100644 index 000000000..bbb220630 --- /dev/null +++ b/src/HelpParser.mjs @@ -0,0 +1,100 @@ +'use strict' + +import fs from 'node:fs' +import path from 'node:path' + +const DOCUMENTATION_SECTIONS = ['description', 'dependencies', 'configuration', 'commands', 'notes', 'author', 'authors', 'examples', 'tags', 'urls'] + +// Private: Check if a line is a comment +function isCommentLine (line) { + return /^(#|\/\/)/.test(line) +} + +// Private: Remove leading comment markers from a line +function removeCommentPrefix (line) { + return line.replace(/^[#/]+\s*/, '') +} + +// Private: Extract header comment block from file content +// Returns an object with { lines: [...], isHeader: false } when header ends +function extractHeaderCommentBlock (block, currentLine) { + if (!block.isHeader) { + return block + } + + if (isCommentLine(currentLine)) { + block.lines.push(removeCommentPrefix(currentLine)) + } else { + block.isHeader = false + } + + return block +} + +// Private: Parse comment lines into documentation sections +// Returns { description, commands, ..., legacyMode } +function parseDocumentationSections (lines, robotName) { + const scriptDocumentation = {} + let currentSection = null + let legacyMode = false + + for (let i = 0; i < lines.length; i++) { + const line = lines[i] + + if (line.toLowerCase() === 'none') { + continue + } + + const nextSection = line.toLowerCase().replace(':', '') + if (DOCUMENTATION_SECTIONS.indexOf(nextSection) !== -1) { + currentSection = nextSection + scriptDocumentation[currentSection] = [] + } else { + if (currentSection) { + scriptDocumentation[currentSection].push(line) + } + } + } + + if (currentSection === null) { + legacyMode = true + scriptDocumentation.commands = [] + for (let i = 0; i < lines.length; i++) { + const line = lines[i] + if (line.match('-')) { + continue + } + + const cleanedLine = line.slice(2, +line.length + 1 || 9e9).replace(/^hubot/i, robotName).trim() + scriptDocumentation.commands.push(cleanedLine) + } + } + + return { scriptDocumentation, legacyMode } +} + +// Public: Parse help documentation from a script file +// filePath - A String path to the file on disk +// robotName - A String name of the robot (used to replace 'hubot' in legacy mode) +// Returns { scriptDocumentation, commands, legacyMode } +export function parseHelp (filePath, robotName) { + const body = fs.readFileSync(path.resolve(filePath), 'utf-8') + + // Extract header comment block + const useStrictHeaderRegex = /^["']use strict['"];?\s+/ + const lines = body.replace(useStrictHeaderRegex, '').split(/(?:\n|\r\n|\r)/) + .reduce(extractHeaderCommentBlock, { lines: [], isHeader: true }).lines + .filter(Boolean) // remove empty lines + + // Parse sections from header comments + const { scriptDocumentation, legacyMode } = parseDocumentationSections(lines, robotName) + + // Extract commands list + const commands = scriptDocumentation.commands || [] + + return { scriptDocumentation, commands, legacyMode } +} + +export default { + parseHelp +} diff --git a/src/Middleware.mjs b/src/Middleware.mjs index fd218762a..88f5808ad 100644 --- a/src/Middleware.mjs +++ b/src/Middleware.mjs @@ -29,6 +29,18 @@ class Middleware { return shouldContinue } + // Public: Cheap Design helper: Execute middleware and return true only if + // middleware did not explicitly return false. Simplifies the common + // "execute and check" pattern across the codebase. + // + // context - context object to pass through the middleware stack + // + // Returns true if middleware allows continuation, false otherwise + async executeAndAllow (context) { + const result = await this.execute(context) + return result !== false + } + // Public: Registers new middleware // // middleware - Middleware function to execute prior to the listener callback. Return false to prevent execution of the listener callback. diff --git a/src/Response.mjs b/src/Response.mjs index c1b0b22e2..a21c02b7a 100644 --- a/src/Response.mjs +++ b/src/Response.mjs @@ -1,5 +1,16 @@ 'use strict' +// Cheap Design: Define response method variations as data, not code. +// Each entry maps method name to options; methods are generated dynamically. +const RESPONSE_METHODS = { + send: { plaintext: true }, + emote: { plaintext: true }, + reply: { plaintext: true }, + topic: { plaintext: true }, + play: {}, + locked: { plaintext: true } +} + class Response { // Public: Responses are sent to matching listeners. Messages know about the // content and user that made the original message, and how to reply back to @@ -25,63 +36,11 @@ class Response { // should be kept intact. // // Returns result from middleware. - async send (...strings) { - return await this.#runWithMiddleware('send', { plaintext: true }, ...strings) - } - - // Public: Posts an emote back to the chat source - // - // strings - One or more strings to be posted. The order of these strings - // should be kept intact. - // - // Returns result from middleware. - async emote (...strings) { - return await this.#runWithMiddleware('emote', { plaintext: true }, ...strings) - } - - // Public: Posts a message mentioning the current user. - // - // strings - One or more strings to be posted. The order of these strings - // should be kept intact. - // - // Returns result from middleware. - async reply (...strings) { - return await this.#runWithMiddleware('reply', { plaintext: true }, ...strings) - } - - // Public: Posts a topic changing message - // - // strings - One or more strings to set as the topic of the - // room the bot is in. - // - // Returns result from middleware. - async topic (...strings) { - return await this.#runWithMiddleware('topic', { plaintext: true }, ...strings) - } - - // Public: Play a sound in the chat source - // - // strings - One or more strings to be posted as sounds to play. The order of - // these strings should be kept intact. - // - // Returns result from middleware. - async play (...strings) { - return await this.#runWithMiddleware('play', {}, ...strings) - } - - // Public: Posts a message in an unlogged room - // - // strings - One or more strings to be posted. The order of these strings - // should be kept intact. - // - // Returns result from middleware. - async locked (...strings) { - await this.#runWithMiddleware('locked', { plaintext: true }, ...strings) - } + // Note: This method is dynamically generated from RESPONSE_METHODS registry - // Call with a method for the given strings using response - // middleware. - async #runWithMiddleware (methodName, opts, ...strings) { + // Internal: Run the given method through response middleware and adapter. + // Used by dynamically-generated response methods (send, emote, reply, etc). + async _runWithMiddleware (methodName, opts, ...strings) { const context = { response: this, strings, @@ -92,8 +51,9 @@ class Response { context.plaintext = true } - const shouldContinue = await this.robot.middleware.response.execute(context) - if (shouldContinue === false) return + // Cheap Design: Use Middleware.executeAndAllow() for cleaner conditional + const shouldContinue = await this.robot.middleware.response.executeAndAllow(context) + if (!shouldContinue) return return await this.robot.adapter[methodName](this.envelope, ...context.strings) } @@ -125,3 +85,15 @@ class Response { } export default Response + +// Cheap Design: Generate all response methods from the registry. +// This eliminates 60+ lines of duplicate method definitions. Each method +// is generated once by mapping over RESPONSE_METHODS and creating an +// async function that delegates to _runWithMiddleware with the appropriate +// method name and options. Adding new response types requires only adding +// an entry to the registry, not writing a new method. +Object.entries(RESPONSE_METHODS).forEach(([methodName, opts]) => { + Response.prototype[methodName] = async function (...strings) { + return await this._runWithMiddleware(methodName, opts, ...strings) + } +}) diff --git a/src/Robot.mjs b/src/Robot.mjs index 91147014f..32515c5c7 100644 --- a/src/Robot.mjs +++ b/src/Robot.mjs @@ -11,10 +11,10 @@ import Response from './Response.mjs' import { Listener, TextListener } from './Listener.mjs' import Message from './Message.mjs' import Middleware from './Middleware.mjs' +import { parseHelp } from './HelpParser.mjs' const File = fs.promises const HUBOT_DEFAULT_ADAPTERS = ['Campfire', 'Shell'] -const HUBOT_DOCUMENTATION_SECTIONS = ['description', 'dependencies', 'configuration', 'commands', 'notes', 'author', 'authors', 'examples', 'tags', 'urls'] const __filename = fileURLToPath(import.meta.url) const __dirname = path.dirname(__filename) @@ -68,6 +68,7 @@ class Robot { this.parseVersion() this.errorHandlers = [] + this.catchAllListenerExecuted = false this.on('error', (err, res) => { return this.invokeErrorHandlers(err, res) @@ -75,6 +76,29 @@ class Robot { this.on('listening', this.herokuKeepalive.bind(this)) } + // Internal: Listener registry mapping predefined message type keys to their matcher predicates. + // Used by _registerListener() to consolidate similar listener registration methods. + static LISTENER_REGISTRY = { + enter: msg => msg instanceof Message.EnterMessage, + leave: msg => msg instanceof Message.LeaveMessage, + topic: msg => msg instanceof Message.TopicMessage + } + + // Internal: Register a listener using a predefined matcher predicate from LISTENER_REGISTRY. + // + // registryKey - A String key into LISTENER_REGISTRY (e.g., 'enter', 'leave', 'topic'). + // options - An Object of additional parameters keyed on extension name (optional). + // callback - A Function that is called with a Response object. + // + // Returns nothing. + _registerListener (registryKey, options, callback) { + const predicate = Robot.LISTENER_REGISTRY[registryKey] + if (!predicate) { + throw new Error(`Unknown listener registry key: ${registryKey}`) + } + this.listen(predicate, options, callback) + } + // Public: Adds a custom Listener with the provided matcher, options, and // callback // @@ -160,7 +184,7 @@ class Robot { // // Returns nothing. enter (options, callback) { - this.listen(msg => msg instanceof Message.EnterMessage, options, callback) + this._registerListener('enter', options, callback) } // Public: Adds a Listener that triggers when anyone leaves the room. @@ -171,7 +195,7 @@ class Robot { // // Returns nothing. leave (options, callback) { - this.listen(msg => msg instanceof Message.LeaveMessage, options, callback) + this._registerListener('leave', options, callback) } // Public: Adds a Listener that triggers when anyone changes the topic. @@ -182,7 +206,7 @@ class Robot { // // Returns nothing. topic (options, callback) { - this.listen(msg => msg instanceof Message.TopicMessage, options, callback) + this._registerListener('topic', options, callback) } // Public: Adds an error handler when an uncaught exception or user emitted @@ -229,6 +253,10 @@ class Robot { options = {} } + // Cheap Design: Register catch-all as a normal listener. + // No special handling in processListeners—it competes with other listeners + // but is only matched when message type is CatchAllMessage (which only happens + // when no other listener matched). This eliminates the recursive fallback. this.listen(isCatchAllMessage, options, async msg => { await callback(msg) }) @@ -283,8 +311,10 @@ class Robot { // Returns array of results from listeners. async receive (message) { const context = { response: new Response(this, message) } - const shouldContinue = await this.middleware.receive.execute(context) - if (shouldContinue === false) return null + // Cheap Design: Use Middleware.executeAndAllow() to simplify the + // middleware invocation and conditional check into a single semantic call + const shouldContinue = await this.middleware.receive.executeAndAllow(context) + if (!shouldContinue) return null return await this.processListeners(context) } @@ -316,45 +346,52 @@ class Robot { } } + // Cheap Design: Replace recursive receive() with simple message type conversion. + // Let the listener table handle catch-all uniformly by just changing the message type. + // No special fallback logic, no recursion—just data transformation followed by + // a second pass through the same listener loop. Catch-all listeners match via + // isCatchAllMessage predicate, so this naturally triggers them in the loop above. if (!isCatchAllMessage(context.response.message) && !anyListenersExecuted) { - this.logger.debug('No listeners executed; falling back to catch-all') - try { - const result = await this.receive(new Message.CatchAllMessage(context.response.message)) - results.push(result) - } catch (err) { - this.emit('error', err, context) - } + this.logger.debug('No listeners executed; converting to catch-all message') + const catchAllMsg = new Message.CatchAllMessage(context.response.message) + context.response.message = catchAllMsg + // Re-process listeners with the converted message type + return await this.processListeners(context) } return results } - async loadmjs (filePath) { + // Private: Generic loader applying Cheap Design by consolidating + // duplicated extension-specific logic. Offloads variation to data (ext) + // while keeping a single control path. + async _loadScript (filePath, ext) { const forImport = this.prepareForImport(filePath) - const script = await import(forImport) - let result = null - if (typeof script?.default === 'function') { - result = await script.default(this) - } else { - this.logger.warn(`Expected ${filePath} (after preparing for import ${forImport}) to assign a function to export default, got ${typeof script}`) + let exported + try { + exported = await import(forImport) + } catch (err) { + this.logger.error(`Import failed for ${filePath}: ${err.stack}`) + throw err } - return result + const scriptFn = exported?.default + if (typeof scriptFn === 'function') { + return await scriptFn(this) + } + this.logger.warn(`Expected ${filePath} (${ext}) default export to be function, got ${typeof scriptFn}`) + return null + } + + async loadmjs (filePath) { + return await this._loadScript(filePath, 'mjs') } async loadts (filePath) { - return this.loadmjs(filePath) + return await this._loadScript(filePath, 'ts') } async loadjs (filePath) { - const forImport = this.prepareForImport(filePath) - const script = (await import(forImport)).default - let result = null - if (typeof script === 'function') { - result = await script(this) - } else { - this.logger.warn(`Expected ${filePath} (after preparing for import ${forImport}) to assign a function to module.exports, got ${typeof script}`) - } - return result + return await this._loadScript(filePath, 'js') } // Public: Loads a file in path. @@ -374,6 +411,7 @@ class Robot { } let result = null try { + // Cheap Design: remove branching differences, rely on consolidated loader result = await this[`load${ext}`](full) this.parseHelp(full) } catch (error) { @@ -507,17 +545,11 @@ class Robot { return } this.logger.debug(`Loading adapter ${adapterPath ?? 'from npmjs:'} ${this.adapterName}`) - const ext = path.extname(adapterPath ?? '') try { - if (Array.from(HUBOT_DEFAULT_ADAPTERS).indexOf(this.adapterName) > -1) { - this.adapter = await this.requireAdapterFrom(path.resolve(path.join(__dirname, 'adapters', `${this.adapterName}.mjs`))) - } else if (['.js', '.cjs'].includes(ext)) { - this.adapter = await this.requireAdapterFrom(path.resolve(adapterPath)) - } else if (['.mjs'].includes(ext)) { - this.adapter = await this.importAdapterFrom(path.resolve(adapterPath)) - } else { - this.adapter = await this.importFromRepo(this.adapterName) - } + // Cheap Design: Resolve strategy table driven by adapter state/path, + // eliminating nested if/else branching. Each strategy is a simple resolver. + const resolver = this._selectAdapterResolver(adapterPath) + this.adapter = await resolver.call(this, adapterPath) } catch (error) { this.logger.error(`Cannot load adapter ${adapterPath ?? '[no path set]'} ${this.adapterName} - ${error}`) throw error @@ -526,17 +558,52 @@ class Robot { this.adapterName = this.adapter.name ?? this.adapter.constructor.name } - async requireAdapterFrom (adapaterPath) { - return await this.importAdapterFrom(adapaterPath) + // Private: Select the appropriate adapter resolver based on path and adapter name. + // Returns a resolver function that loads and returns the adapter. + _selectAdapterResolver (adapterPath) { + const ext = path.extname(adapterPath ?? '') + const isBuiltin = Array.from(HUBOT_DEFAULT_ADAPTERS).indexOf(this.adapterName) > -1 + + // Resolver table: condition => resolver function + const resolvers = { + builtinAdapter: () => isBuiltin, + commonJsFile: () => ['.js', '.cjs'].includes(ext), + esModuleFile: () => ['.mjs'].includes(ext), + npmPackage: () => true // fallback + } + + const resolverMap = { + builtinAdapter: this._loadBuiltinAdapter, + commonJsFile: this._loadLocalAdapter, + esModuleFile: this._loadLocalAdapter, + npmPackage: this._loadNpmAdapter + } + + // Find the first matching condition and return its resolver + for (const [key, condition] of Object.entries(resolvers)) { + if (condition()) { + return resolverMap[key] + } + } + } + + // Private: Load a built-in adapter from src/adapters/ + async _loadBuiltinAdapter (adapterPath) { + const builtinPath = path.resolve(path.join(__dirname, 'adapters', `${this.adapterName}.mjs`)) + const forImport = this.prepareForImport(builtinPath) + return await (await import(forImport)).default.use(this) } - async importAdapterFrom (adapterPath) { - const forImport = this.prepareForImport(adapterPath) + // Private: Load a local adapter from a file path. + async _loadLocalAdapter (adapterPath) { + const resolvedPath = path.resolve(adapterPath) + const forImport = this.prepareForImport(resolvedPath) return await (await import(forImport)).default.use(this) } - async importFromRepo (adapterPath) { - return await (await import(adapterPath)).default.use(this) + // Private: Load an npm package adapter. + async _loadNpmAdapter (adapterPath) { + return await (await import(this.adapterName)).default.use(this) } // Public: Help Commands for Running Scripts. @@ -552,52 +619,22 @@ class Robot { // // Returns nothing. parseHelp (filePath) { - const scriptDocumentation = {} - const body = fs.readFileSync(path.resolve(filePath), 'utf-8') - - const useStrictHeaderRegex = /^["']use strict['"];?\s+/ - const lines = body.replace(useStrictHeaderRegex, '').split(/(?:\n|\r\n|\r)/) - .reduce(toHeaderCommentBlock, { lines: [], isHeader: true }).lines - .filter(Boolean) // remove empty lines - let currentSection = null - let nextSection - + // Cheap Design: Extracted help parsing into a dedicated utility module. + // Robot no longer needs to understand comment extraction or documentation + // section parsing—it just delegates to HelpParser and collects commands. this.logger.debug(`Parsing help for ${filePath}`) - for (let i = 0, line; i < lines.length; i++) { - line = lines[i] - - if (line.toLowerCase() === 'none') { - continue - } + try { + const { commands, legacyMode } = parseHelp(filePath, this.name) - nextSection = line.toLowerCase().replace(':', '') - if (Array.from(HUBOT_DOCUMENTATION_SECTIONS).indexOf(nextSection) !== -1) { - currentSection = nextSection - scriptDocumentation[currentSection] = [] - } else { - if (currentSection) { - scriptDocumentation[currentSection].push(line) - if (currentSection === 'commands') { - this.commands.push(line) - } - } + if (legacyMode) { + this.logger.info(`${filePath} is using deprecated documentation syntax`) } - } - if (currentSection === null) { - this.logger.info(`${filePath} is using deprecated documentation syntax`) - scriptDocumentation.commands = [] - for (let i = 0, line, cleanedLine; i < lines.length; i++) { - line = lines[i] - if (line.match('-')) { - continue - } - - cleanedLine = line.slice(2, +line.length + 1 || 9e9).replace(/^hubot/i, this.name).trim() - scriptDocumentation.commands.push(cleanedLine) - this.commands.push(cleanedLine) - } + // Collect commands from this script + this.commands.push(...commands) + } catch (error) { + this.logger.error(`Failed to parse help for ${filePath}: ${error.stack}`) } } @@ -756,28 +793,6 @@ function isCatchAllMessage (message) { return message instanceof Message.CatchAllMessage } -function toHeaderCommentBlock (block, currentLine) { - if (!block.isHeader) { - return block - } - - if (isCommentLine(currentLine)) { - block.lines.push(removeCommentPrefix(currentLine)) - } else { - block.isHeader = false - } - - return block -} - -function isCommentLine (line) { - return /^(#|\/\/)/.test(line) -} - -function removeCommentPrefix (line) { - return line.replace(/^[#/]+\s*/, '') -} - function extend (obj, ...sources) { sources.forEach((source) => { if (typeof source !== 'object') { diff --git a/src/adapters/Campfire.mjs b/src/adapters/Campfire.mjs index 49d02cc51..6afa22a4a 100644 --- a/src/adapters/Campfire.mjs +++ b/src/adapters/Campfire.mjs @@ -6,6 +6,11 @@ import Adapter from '../Adapter.mjs' import { TextMessage, EnterMessage, LeaveMessage, TopicMessage } from '../Message.mjs' class Campfire extends Adapter { + // Campfire-specific formatters for message transformation + #emoteFormatter = (str, envelope) => `*${str}*` + + #replyFormatter = (str, envelope) => `${envelope.user.name}: ${str}` + send (envelope/* , ...strings */) { const strings = [].slice.call(arguments, 1) @@ -28,15 +33,9 @@ class Campfire extends Adapter { }) } - emote (envelope/* , ...strings */) { - const strings = [].slice.call(arguments, 1) - this.send.apply(this, [envelope].concat(strings.map(str => `*${str}*`))) - } + emote = this._createSendDelegate(this.#emoteFormatter) - reply (envelope/* , ...strings */) { - const strings = [].slice.call(arguments, 1) - this.send.apply(this, [envelope].concat(strings.map(str => `${envelope.user.name}: ${str}`))) - } + reply = this._createSendDelegate(this.#replyFormatter) topic (envelope/* , ...strings */) { const strings = [].slice.call(arguments, 1) diff --git a/src/adapters/Shell.mjs b/src/adapters/Shell.mjs index 5d9939274..cddbce82d 100644 --- a/src/adapters/Shell.mjs +++ b/src/adapters/Shell.mjs @@ -71,14 +71,14 @@ class Shell extends Adapter { Array.from(strings).forEach(str => console.log(bold(str))) } - async emote (envelope, ...strings) { - Array.from(strings).map(str => this.send(envelope, `* ${str}`)) - } + // Shell-specific formatters for message transformation + #emoteFormatter = (str, envelope) => `* ${str}` - async reply (envelope, ...strings) { - strings = strings.map((s) => `${envelope.user.name}: ${s}`) - await this.send(envelope, ...strings) - } + #replyFormatter = (str, envelope) => `${envelope.user.name}: ${str}` + + emote = this._createSendDelegate(this.#emoteFormatter) + + reply = this._createSendDelegate(this.#replyFormatter) async run () { try {