Refactor settings architecture and remove singleton pattern
Reworked `Settings` class to replace the static singleton pattern with constructor injection of `SettingsProvider`, improving testability and modularity. Updated all affected classes, tests, and configurations to align with the new architecture.
This commit is contained in:
1 parent
1d737a2a4f
commit
64f4890931
21 files changed
+170
-120
No files matched your search
@@ -76,7 +76,7 @@ Improve the code quality, correctness, and maintainability of `QueueManager`, `T
|
||||
All changes in `src/main/java/me/bobert/netneutrality/music/`:
|
||||
- **Modified:** `QueueManager.java`, `TrackScheduler.java`, `TrackInfo.java`, `VoteSkipService.java`
|
||||
- **New:** `StuckTrackRecoveryHandler.java`
|
||||
- **New tests:** `src/test/java/me/bobert/netneutrality/feature/music/QueueManagerTest.java`, `TrackSchedulerTest.java`, `TrackInfoTest.java`, `VoteSkipServiceTest.java`
|
||||
- **New tests:** `src/test/java/me/bobert/netneutrality/command/music/QueueManagerTest.java`, `TrackSchedulerTest.java`, `TrackInfoTest.java`, `VoteSkipServiceTest.java`
|
||||
|
||||
# Testing
|
||||
|
||||
|
||||
@@ -0,0 +1,91 @@
|
||||
---
|
||||
sessionId: session-260909-154047-1h6n
|
||||
---
|
||||
|
||||
# Requirements
|
||||
|
||||
### Overview & Goals
|
||||
Remove the singleton pattern from `Settings` so it becomes a plain instantiable class. All access should go through constructor injection of `SettingsProvider`, except for the single bootstrap site in `App.main()` which creates the instance.
|
||||
|
||||
### Scope
|
||||
**In Scope:**
|
||||
- Remove `static INSTANCE` field and `getInstance()` from `Settings`
|
||||
- Remove static helper methods `getDiscordInvite()` and `getVersion()` from `Settings`
|
||||
- Migrate all remaining `Settings.getInstance()` call sites to use injected `SettingsProvider`
|
||||
- Update the test to construct `Settings` directly
|
||||
|
||||
**Out of Scope:**
|
||||
- Changing the `SettingsProvider` interface
|
||||
- Introducing a DI framework
|
||||
- Changing how properties are loaded
|
||||
|
||||
# Technical Design
|
||||
|
||||
### Current Implementation
|
||||
- `Settings` is a lazy singleton (`getInstance()`) implementing `SettingsProvider`
|
||||
- `SettingsProvider` interface already exists and is injected into most classes via `ApplicationContext`
|
||||
- `App.buildContext()` already registers the Settings instance as `SettingsProvider` in the context
|
||||
- **Remaining direct singleton usages (9 call sites):**
|
||||
- `App.java` — `VERSION` static field (line 51), `main()` bootstrap (lines 61-62), `startup()` (line 89), `buildContext()` (line 112), `initJDA()` (line 190)
|
||||
- `CommandContext.java` — `isMessageFromCreator()` (line 134)
|
||||
- `WebModule.java` — `createRequest()` (line 289) via `Settings.getVersion()`
|
||||
- `ReportBug.java` — `executeTask()` (line 34) via `Settings.getDiscordInvite()`
|
||||
- `SettingsTest.java` — `getMavenVersion()` test (line 17)
|
||||
|
||||
### Proposed Changes
|
||||
|
||||
#### 1. `Settings.java`
|
||||
- Make constructor `public`
|
||||
- Remove `static INSTANCE` field, `getInstance()`, `getDiscordInvite()`, `getVersion()`
|
||||
- Keep `getMavenVersion()` as an instance method
|
||||
- Add `getDiscordInvite()` as an instance method on `SettingsProvider` (or keep it as a convenience on `Settings`)
|
||||
|
||||
#### 2. `App.java`
|
||||
- Create `Settings` via `new Settings()` at the top of `main()`; pass it into `App` constructor or store as a field
|
||||
- Replace `VERSION` static field with an instance field initialized from the settings instance
|
||||
- Thread the `SettingsProvider` through `startup()`, `buildContext()`, `initJDA()`
|
||||
|
||||
#### 3. `CommandContext.java`
|
||||
- Add a `SettingsProvider` field, accept it via constructor parameter
|
||||
- Update `isMessageFromCreator()` to use the injected provider
|
||||
- Update subclasses (`MessageCommandContext`, `SlashCommandContext`, etc.) to pass it through
|
||||
|
||||
#### 4. `WebModule.java`
|
||||
- Accept a version string or `SettingsProvider` in the `Builder`; replace `Settings.getVersion()` call
|
||||
|
||||
#### 5. `ReportBug.java`
|
||||
- Accept `SettingsProvider` via constructor injection; replace `Settings.getDiscordInvite()` with `settings.get("NETNEUTRALITY_BOT_INVITE", ...)`
|
||||
|
||||
#### 6. `SettingsTest.java`
|
||||
- Use `new Settings()` instead of `Settings.getInstance()`
|
||||
|
||||
### Risks
|
||||
- `CommandContext` is abstract with multiple subclasses — all constructors need updating
|
||||
- `WebModule.Builder` is used in many places including inside `Settings.getGitVersion()` itself — the version string approach (pass a `String` rather than the full provider) keeps this simpler
|
||||
|
||||
# Delivery Steps
|
||||
|
||||
### ✓ Step 1: Remove singleton from Settings class
|
||||
Settings can be instantiated with `new Settings()` and no longer holds static state.
|
||||
|
||||
- Make the `Settings` constructor `public`
|
||||
- Remove the `static INSTANCE` field and `getInstance()` method
|
||||
- Remove static helpers `getDiscordInvite()` and `getVersion()`
|
||||
- Move `getDiscordInvite()` logic to an instance method (or inline at call sites)
|
||||
- Keep `getMavenVersion()` as-is (already instance method)
|
||||
|
||||
### ✓ Step 2: Migrate App.java bootstrap to explicit instantiation
|
||||
App creates and threads a single Settings instance through its lifecycle methods.
|
||||
|
||||
- Create `new Settings()` at the start of `main()` and store as a field on `App`
|
||||
- Replace `Settings.getInstance()` calls in `main()`, `startup()`, `buildContext()`, `initJDA()` with the field
|
||||
- Replace the `VERSION` static field with an instance field or a local derived from the settings instance
|
||||
|
||||
### ✓ Step 3: Inject SettingsProvider into remaining call sites
|
||||
CommandContext, WebModule, and ReportBug no longer reach for the singleton.
|
||||
|
||||
- Add `SettingsProvider` constructor parameter to `CommandContext`; update `isMessageFromCreator()` to use it
|
||||
- Update `CommandContext` subclasses (`MessageCommandContext`, `SlashCommandContext`, etc.) to pass the provider through
|
||||
- In `WebModule.Builder`, accept a version string parameter; replace `Settings.getVersion()` in `createRequest()`
|
||||
- In `ReportBug`, add `SettingsProvider` constructor parameter; replace `Settings.getDiscordInvite()` with `settings.get(...)`
|
||||
- Update `SettingsTest` to use `new Settings()` directly
|
||||
Reference in new issue
Block a user