-
Notifications
You must be signed in to change notification settings - Fork 3
Closed as not planned
Closed as not planned
Copy link
Description
🔍 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
-
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
- Since all loggers already implement
-
Optional: Add PreClose Hook Support
- If needed in the future, add optional
PreClose() errormethod 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
- If needed in the future, add optional
-
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