[duplicate-code] Duplicate Code Pattern: Logger Close() Method Duplication

[github-actions]

🔍 Duplicate Code Pattern: Logger Close() Method Duplication

Part of duplicate code analysis: #435

Summary

The three logger types implement nearly identical Close() methods that follow the same pattern: acquire mutex, call closeLogFile() helper, release mutex. While the duplication is minimal (6-8 lines per method), it represents an opportunity for interface-based abstraction.

Duplication Details

Pattern: Logger Close() Methods

• Severity: Medium
• Occurrences: 3 instances (FileLogger, JSONLLogger, MarkdownLogger)
• Locations:
  • internal/logger/file_logger.go (lines 58-62, 5 lines)
  • internal/logger/jsonl_logger.go (lines 59-63, 5 lines)
  • internal/logger/markdown_logger.go (lines 67-80, 14 lines)

Code Comparison:

FileLogger.Close() (5 lines):

func (fl *FileLogger) Close() error {
    fl.mu.Lock()
    defer fl.mu.Unlock()
    return closeLogFile(fl.logFile, &fl.mu, "file")
}

JSONLLogger.Close() (5 lines):

func (jl *JSONLLogger) Close() error {
    jl.mu.Lock()
    defer jl.mu.Unlock()
    return closeLogFile(jl.logFile, &jl.mu, "JSONL")
}

MarkdownLogger.Close() (14 lines - more complex):

func (ml *MarkdownLogger) Close() error {
    ml.mu.Lock()
    defer ml.mu.Unlock()

    if ml.logFile != nil {
        // Write closing details tag before closing
        footer := "\n</details>\n"
        if _, err := ml.logFile.WriteString(footer); err != nil {
            // Even if footer write fails, try to close the file properly
            return closeLogFile(ml.logFile, &ml.mu, "markdown")
        }

        // Footer written successfully, now close
        return closeLogFile(ml.logFile, &ml.mu, "markdown")
    }
    return nil
}

Note: MarkdownLogger is an exception - it needs special cleanup logic to write the footer before closing.

Impact Analysis

Maintainability

• Low impact: Only 3 instances, pattern is clear and well-established
• Common pattern in Go: Wrapper methods around shared helpers are idiomatic
• Already refactored: The closeLogFile() helper in common.go (lines 23-36) consolidates the actual closing logic

Bug Risk

• Very low: The pattern is simple and unlikely to diverge
• Good design: Each logger calls the same closeLogFile() helper, ensuring consistent behavior
• Special cases handled: MarkdownLogger's footer-writing logic is appropriately placed in its Close() method

Code Bloat

• Minimal: ~10 lines total duplication (excluding MarkdownLogger's special logic)
• Not a priority: Given the small size and clear pattern, this is acceptable duplication

Refactoring Recommendations

Option 1: Interface with Default Implementation (Go 1.23+)

Note: This requires Go 1.23+ for interface with methods. Consider carefully before implementing.

// Define a closable logger interface
type ClosableLogger interface {
    GetLogFile() *os.File
    GetMutex() *sync.Mutex
    GetLoggerName() string
    BeforeClose() error  // Hook for custom cleanup
}

// Default Close implementation
func CloseLogger(cl ClosableLogger) error {
    mu := cl.GetMutex()
    mu.Lock()
    defer mu.Unlock()
    
    // Call before-close hook if needed
    if err := cl.BeforeClose(); err != nil {
        return err
    }
    
    return closeLogFile(cl.GetLogFile(), mu, cl.GetLoggerName())
}

// Usage:
func (fl *FileLogger) Close() error {
    return CloseLogger(fl)
}

func (fl *FileLogger) BeforeClose() error {
    return nil  // No special cleanup
}

func (ml *MarkdownLogger) BeforeClose() error {
    if ml.logFile != nil {
        footer := "\n</details>\n"
        _, err := ml.logFile.WriteString(footer)
        return err
    }
    return nil
}

Benefits:

• Eliminates ~10 lines of duplication
• Makes the close pattern explicit and reusable
• Provides clear extension point for logger-specific cleanup

Drawbacks:

• Adds complexity for minimal gain
• Requires additional interface methods
• Less idiomatic than simple wrapper methods in Go

Estimated effort: 2-3 hours (not recommended given low value)

Option 2: Accept the Pattern (Recommended)

Recommendation: Do not refactor - this level of duplication is acceptable and idiomatic in Go.

Rationale:

1. Go idiom: Wrapper methods that add context to shared functions are standard practice
2. Clear and simple: Each Close() method is trivial to understand
3. Low maintenance burden: Unlikely to change or diverge
4. Special cases: MarkdownLogger demonstrates why per-type methods are valuable
5. Already optimized: The shared closeLogFile() helper already eliminates the complex logic
6. Cost-benefit: Refactoring effort outweighs benefits

What has been done well:

• ✅ Shared closeLogFile() helper in common.go (36 lines of reusable logic)
• ✅ Consistent error handling across all Close() methods
• ✅ Proper mutex locking pattern
• ✅ Each logger can customize behavior as needed (MarkdownLogger footer)

Option 3: Document the Pattern

If concerned about consistency, add documentation:

// internal/logger/common.go

// Close Pattern for Logger Types
//
// All logger types should implement Close() using this pattern:
//
//   func (*Logger) Close() error {
//       *.mu.Lock()
//       defer *.mu.Unlock()
//       
//       // Optional: Perform cleanup before closing (e.g., write footer)
//       
//       return closeLogFile(*.logFile, &*.mu, "loggerName")
//   }
//
// The closeLogFile helper handles syncing and closing with proper error handling.

Benefits:

• Makes the pattern explicit for future developers
• Ensures consistency when adding new logger types
• Zero code changes required

Estimated effort: 15 minutes

Implementation Checklist

• Recommended: Accept this pattern as acceptable duplication
• Optional: Add documentation to common.go explaining the Close() pattern
• Not recommended: Implement interface-based abstraction (cost exceeds benefit)

Parent Issue

See parent analysis report: #435
Related to #435

AI generated by Duplicate Code Detector