-
Notifications
You must be signed in to change notification settings - Fork 1
Closed
Labels
Description
Summary
The Docs package repeats a 5-line document_id extraction+validation+URL-parsing block ~16 times across 3 testable files. Similarly, the Sheets package repeats a spreadsheet_id block 7 times. Extracting per-package helpers would eliminate significant boilerplate.
Location
Docs package (~16 instances):
internal/docs/docs_formatting_testable.go: L128-138, L182-192, L227-237, L280-315, L355-365internal/docs/docs_content_testable.go: L48-51, L74-79, L105-110, L141-151, L203-222, L252-268, L308-311, L350-360internal/docs/docs_structure_testable.go: L19-45, L79-102, L158-172, L201-220, L262-267
Sheets package (7 instances):
internal/sheets/sheets_tools_testable.go: L23-27, L69-73, L112-116, L166-170, L252-256, L304-308, L371-375
Category
Type: DRY Violation
Severity: Medium
Evidence
Docs — repeated 16x:
docID, _ := request.Params.Arguments["document_id"].(string)
if docID == "" {
return mcp.NewToolResultError("document_id parameter is required"), nil
}
docID = common.ExtractGoogleResourceID(docID)Sheets — repeated 7x:
spreadsheetID, _ := request.Params.Arguments["spreadsheet_id"].(string)
if spreadsheetID == "" {
return mcp.NewToolResultError("spreadsheet_id parameter is required"), nil
}
spreadsheetID = common.ExtractSpreadsheetID(spreadsheetID)Suggested Refactoring
- Add
extractRequiredDocID(request) (string, *mcp.CallToolResult)in docs package - Add
extractRequiredSpreadsheetID(request) (string, *mcp.CallToolResult)in sheets package - Replace all inline instances with calls to the helpers
- Consider a generic
common.ExtractRequiredArg(request, key, extractFn) (string, *mcp.CallToolResult)if the pattern extends further
Effort Estimate
- Size: Small (< 1 hour)
- Risk: Low (simple extraction)
- Tests Required: Yes (existing tests must pass)
Acceptance Criteria
-
extractRequiredDocIDhelper added and used across all 3 docs testable files -
extractRequiredSpreadsheetIDhelper added and used in sheets testable file - ~23 instances of boilerplate replaced
- All existing tests pass
Reactions are currently unavailable