-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor pkg/trackclipboard to align with SOLID principles #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This commit refactors the trackclipboard module to improve its design and
maintainability by applying SOLID principles.
Key changes:
1. **Single Responsibility Principle (SRP):**
* `local_channel.go`: Extracted file/path manipulation (`ensureLogFile`) from `processMessages`.
* `track.go`:
* `NewTrackClipboard`: Separated configuration defaulting logic (`getDefaultedAPPConfig`, `getDefaultedFileConfig`) from core construction.
* `Track`: Extracted clipboard data handling logic (`handleClipboardData`) from the main event loop.
2. **Open/Closed Principle (OCP):**
* `track.go`: Implemented a factory pattern (`ChannelFactory` and registration) for creating `TrackChannel` instances (local, telegram). This replaces the `switch` statement in `NewTrackClipboard`, allowing new channel types to be added without modifying existing code.
3. **Dependency Inversion Principle (DIP):**
* `TrackClipboard` now depends on the `TrackChannel` interface.
* The factory pattern helps abstract the creation of concrete channel implementations.
4. **Unit Tests:**
* Added comprehensive unit tests for all components within the module.
* Utilized `net/http/httptest` for mocking Telegram API calls in `telegram_channel_test.go`.
* Implemented a `mockTrackChannel` for testing `TrackClipboard` interactions.
* Used package-level function variables to mock `clipboard.Init` and `clipboard.Watch` for testing the `Track` method's orchestration logic.
These changes lead to a more modular, testable, and maintainable `trackclipboard` package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the trackclipboard module to align with SOLID principles by decoupling responsibilities, introducing a factory pattern for channel creation, and improving error handling and test coverage.
- Extracts file/path manipulation and default configuration logic into dedicated functions.
- Introduces a ChannelFactory registry with separate factories for local and Telegram channels.
- Enhances unit tests and refines error messages, particularly in clipboard data handling and file operations.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/trackclipboard/track.go | Refactored configuration defaulting and channel creation with a factory pattern. |
| pkg/trackclipboard/local_channel.go | Updated processMessages to utilize ensureLogFile and improved file handling logic. |
| pkg/trackclipboard/local_channel_test.go | Added comprehensive tests for local channel functionality and error scenarios. |
| } else if strings.HasPrefix(cfg.Path, "~") { // Handle tilde expansion for Path | ||
| homeDir, err := os.UserHomeDir() | ||
| if err != nil { | ||
| panic(fmt.Errorf("error getting user home directory for tilde expansion: %w", err)) | ||
| } | ||
| cfg.Path = filepath.Join(homeDir, cfg.Path[2:]) |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The tilde expansion logic assumes the path always starts with '~/' and strips the first two characters. Consider validating the format or adding a clarifying comment to document this expectation.
| } else if strings.HasPrefix(cfg.Path, "~") { // Handle tilde expansion for Path | |
| homeDir, err := os.UserHomeDir() | |
| if err != nil { | |
| panic(fmt.Errorf("error getting user home directory for tilde expansion: %w", err)) | |
| } | |
| cfg.Path = filepath.Join(homeDir, cfg.Path[2:]) | |
| } else if strings.HasPrefix(cfg.Path, "~") { | |
| // Handle tilde expansion for Path. Expectation: paths starting with '~' must be followed by '/'. | |
| if len(cfg.Path) > 1 && cfg.Path[1] == '/' { | |
| homeDir, err := os.UserHomeDir() | |
| if err != nil { | |
| panic(fmt.Errorf("error getting user home directory for tilde expansion: %w", err)) | |
| } | |
| cfg.Path = filepath.Join(homeDir, cfg.Path[2:]) | |
| } else { | |
| panic(fmt.Errorf("invalid path format for tilde expansion: %s", cfg.Path)) | |
| } |
| fmt.Printf("Error ensuring log file: %v\n", err) | ||
| return | ||
| } | ||
| defer f.Close() | ||
| l.file = f // Store the file handle in the struct |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Rather than only printing the error, consider propagating it or implementing a more robust error handling strategy to flag file initialization failures louder.
| func (l *LocalChannel) Close() error { | ||
| // Signal the processMessages goroutine to stop | ||
| close(l.doneChan) | ||
| if l.doneChan != nil { |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider introducing synchronization (e.g., a WaitGroup) to ensure that the processMessages goroutine has fully terminated before Close() returns, preventing potential race conditions.
This commit refactors the trackclipboard module to improve its design and maintainability by applying SOLID principles.
Key changes:
Single Responsibility Principle (SRP):
local_channel.go: Extracted file/path manipulation (ensureLogFile) fromprocessMessages.track.go:NewTrackClipboard: Separated configuration defaulting logic (getDefaultedAPPConfig,getDefaultedFileConfig) from core construction.Track: Extracted clipboard data handling logic (handleClipboardData) from the main event loop.Open/Closed Principle (OCP):
track.go: Implemented a factory pattern (ChannelFactoryand registration) for creatingTrackChannelinstances (local, telegram). This replaces theswitchstatement inNewTrackClipboard, allowing new channel types to be added without modifying existing code.Dependency Inversion Principle (DIP):
TrackClipboardnow depends on theTrackChannelinterface.Unit Tests:
net/http/httptestfor mocking Telegram API calls intelegram_channel_test.go.mockTrackChannelfor testingTrackClipboardinteractions.clipboard.Initandclipboard.Watchfor testing theTrackmethod's orchestration logic.These changes lead to a more modular, testable, and maintainable
trackclipboardpackage.