-
Notifications
You must be signed in to change notification settings - Fork 3
Closed
Description
🔍 Duplicate Code Pattern: Repeated Error Wrapping Pattern
Part of duplicate code analysis: #351
Summary
The fmt.Errorf("failed to <action>: %w", err) pattern is repeated 30+ times across multiple files with nearly identical structure. While Go's error wrapping is idiomatic, the consistency of the "failed to" prefix and structure indicates an opportunity for standardization.
Duplication Details
Pattern: "Failed to" Error Wrapping
- Severity: Medium
- Occurrences: 30+ instances across the codebase
- Locations:
internal/cmd/root.go(5 instances)internal/config/*.go(15+ instances)internal/mcp/connection.go(5 instances)internal/launcher/launcher.go(3 instances)internal/logger/*.go(3 instances)
Code Samples:
// From internal/cmd/root.go
return fmt.Errorf("failed to load .env file: %w", err)
return fmt.Errorf("failed to load config: %w", err)
return fmt.Errorf("failed to create unified server: %w", err)
return fmt.Errorf("failed to encode configuration: %w", err)
// From internal/config/config.go
return nil, fmt.Errorf("failed to decode TOML: %w", err)
return nil, fmt.Errorf("failed to read stdin: %w", err)
return nil, fmt.Errorf("failed to normalize configuration: %w", err)
return nil, fmt.Errorf("failed to parse JSON: %w", err)
// From internal/mcp/connection.go
return nil, fmt.Errorf("failed to create HTTP request: %w", err)
return nil, fmt.Errorf("failed to connect: %w", err)
return nil, fmt.Errorf("failed to connect with plain JSON-RPC transport: %w", err)
// From internal/launcher/launcher.go
return nil, fmt.Errorf("failed to create HTTP connection: %w", err)
return nil, fmt.Errorf("failed to create connection: %w", err)Impact Analysis
- Maintainability: While each instance is simple, changing error format requires updates across 30+ locations
- Bug Risk: Low - error wrapping is straightforward
- Code Bloat: Minimal - each instance is 1 line
- Consistency: Medium risk - inconsistent error messages across the codebase
Refactoring Recommendations
1. Error Wrapping Utility Package (Recommended)
- Create consistent error wrapping utilities
- Location:
internal/errors/wrap.go(new package) - Estimated effort: 2-3 hours
- Benefits:
- Consistent error formatting across the codebase
- Easier to add structured logging or error tracking
- Can add context like operation names, stack traces, etc.
- Future-proof for error handling improvements
Example Implementation:
// internal/errors/wrap.go
package errors
import (
"fmt"
)
// Wrap wraps an error with context about what operation failed
func Wrap(err error, operation string) error {
if err == nil {
return nil
}
return fmt.Errorf("failed to %s: %w", operation, err)
}
// Wrapf wraps an error with a formatted context message
func Wrapf(err error, format string, args ...interface{}) error {
if err == nil {
return nil
}
context := fmt.Sprintf(format, args...)
return fmt.Errorf("failed to %s: %w", context, err)
}
// New creates a new error with "failed to" prefix
func New(operation string) error {
return fmt.Errorf("failed to %s", operation)
}Usage Examples:
// Before:
return fmt.Errorf("failed to load config: %w", err)
// After:
return errors.Wrap(err, "load config")
// Before:
return nil, fmt.Errorf("failed to create HTTP connection: %w", err)
// After:
return nil, errors.Wrap(err, "create HTTP connection")2. Alternative: Standard Library Only
- Keep using
fmt.Errorfbut establish conventions in documentation - Add linter rules to enforce consistent error message format
- Lower effort but less enforceable
3. Enhanced Error Package with Context
- Include additional context like operation type, resource names
- Support for structured error logging
- Higher effort but better observability
// Enhanced version with context
type ContextualError struct {
Operation string
Resource string
Cause error
}
func (e *ContextualError) Error() string {
if e.Resource != "" {
return fmt.Sprintf("failed to %s for %s: %v", e.Operation, e.Resource, e.Cause)
}
return fmt.Sprintf("failed to %s: %v", e.Operation, e.Cause)
}
func (e *ContextualError) Unwrap() error {
return e.Cause
}Implementation Checklist
- Create
internal/errorspackage - Implement
Wrap,Wrapf, andNewfunctions - Add unit tests for error wrapping
- Document error handling conventions
- Gradually refactor high-priority files (start with
internal/cmd/root.go) - Consider adding linter rules for error message consistency
- Run full test suite to verify error messages still work
Notes
- Priority: Lower than authorization and transport patterns
- Risk: Very low - error wrapping refactoring is safe
- Incremental: Can be done gradually without breaking changes
- Consider: This pattern is somewhat idiomatic in Go - refactoring may or may not improve clarity
Parent Issue
See parent analysis report: #351
Related to #351
AI generated by Duplicate Code Detector