Skip to content

[duplicate-code] Duplicate Code Pattern: Close Method Implementation #252

@github-actions

Description

@github-actions

🔍 Duplicate Code Pattern: Close Method Implementation

Part of duplicate code analysis: #249

Summary

Three logger types implement nearly identical Close() methods that follow the same pattern: lock mutex, defer unlock, delegate to closeLogFile(). The only variation is the MarkdownLogger which adds a footer before closing. This is acceptable duplication but could be slightly improved.

Duplication Details

Pattern: Close Method Structure

  • Severity: Medium (Low priority - acceptable pattern)
  • Occurrences: 3 instances
  • Locations:
    • internal/logger/jsonl_logger.go (lines 66-72)
    • internal/logger/markdown_logger.go (lines 75-91)
    • internal/logger/file_logger.go (lines 65-71)

Code Sample - JSONLLogger:

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

Code Sample - FileLogger:

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

Code Sample - MarkdownLogger (with footer):

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
}

Impact Analysis

  • Maintainability: Medium - changes to close logic propagation are limited by shared closeLogFile() helper
  • Bug Risk: Low - the duplication is minimal and shared helper reduces risk
  • Code Bloat: ~15 lines of simple delegation code
  • Note: This is a common Go pattern and may not require refactoring

Refactoring Recommendations

  1. Optional: Define io.Closer Interface Pattern

    • Since all loggers already implement Close() error, this is already the standard Go pattern
    • No changes needed unless you want to add pre-close hooks
    • Estimated effort: N/A
    • Benefits: This is already idiomatic Go
  2. Optional: Add PreClose Hook Support

    • If needed in the future, add optional PreClose() error method to logger structs
    • closeLogFile() could check for this method and call it
    • This would eliminate the special case in MarkdownLogger
    • Estimated effort: 1 hour
    • Benefits:
      • Consistent close pattern across all loggers
      • MarkdownLogger footer handling becomes a pre-close hook
      • Easier to add similar behavior to other loggers
  3. Note: Consider Not Refactoring

    • This pattern is minimal, idiomatic Go code
    • The duplication is only 5-6 lines per logger
    • Refactoring might add complexity without significant benefit
    • Recommendation: Only refactor if adding similar pre-close behavior to other loggers

Implementation Checklist

  • Review whether this duplication is worth addressing
  • If yes, design pre-close hook mechanism
  • Implement hook support in closeLogFile()
  • Refactor MarkdownLogger to use pre-close hook
  • Update tests to verify behavior unchanged

Note: This issue has lower priority than the other duplication patterns. Consider addressing only if implementing similar pre-close logic in other loggers.

Parent Issue

See parent analysis report: #249
Related to #249

AI generated by Duplicate Code Detector

Metadata

Metadata

Assignees

No one assigned

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions