95 lines
5.3 KiB
Markdown
95 lines
5.3 KiB
Markdown
---
|
||
sessionId: session-260905-173932-36te
|
||
---
|
||
|
||
# Requirements
|
||
|
||
### Overview & Goals
|
||
Improve the `MusicManager` class for better maintainability, testability, and separation of concerns.
|
||
|
||
### Scope
|
||
**In Scope:**
|
||
- Refactor `MusicManager` to reduce responsibilities
|
||
- Extract vote-skip logic into a dedicated service
|
||
- Fix synchronization strategy
|
||
- Improve error handling
|
||
- Eliminate magic numbers
|
||
|
||
**Out of Scope:**
|
||
- Changing `QueueManager` or `TrackScheduler` internals (beyond reducing circular refs)
|
||
- Changing external behavior or command interfaces
|
||
|
||
# Technical Design
|
||
|
||
### Current Issues
|
||
|
||
1. **Vote-skip business logic in MusicManager** (lines 113–145): The `voteSkip()` method contains complex voting logic (threshold calculation, member counting, skip decisions) that doesn't belong in a player manager. Magic number `2.1` is used as a vote divisor with no explanation.
|
||
|
||
2. **Circular dependencies**: `MusicManager` → `TrackScheduler` → `QueueManager` → `TrackScheduler` → `MusicManager`. This makes testing and reasoning about the code difficult.
|
||
|
||
3. **Overly broad `synchronized` methods**: Nearly every public method is `synchronized` on `this`, even simple getters like `getCurrentlyPlaying()` and `isCurrentlyPlaying()`. This creates unnecessary contention. Methods like `getQueueSize()` aren't synchronized at all, creating inconsistency.
|
||
|
||
4. **Poor error handling in `connect()`** (line 80): Throws a bare `RuntimeException` when `audioChannel` is null instead of using `IllegalArgumentException` or returning a `CommandExecutionResult`.
|
||
|
||
5. **`baseVolume` parsed from String without error handling** (line 44): `Integer.parseInt()` can throw `NumberFormatException` with no fallback.
|
||
|
||
### Proposed Changes
|
||
|
||
#### 1. Extract `VoteSkipService`
|
||
- New class: `me.bobert.netneutrality.music.VoteSkipService`
|
||
- Moves all vote-skip logic out of `MusicManager`
|
||
- Replace magic `2.1` with a named constant `VOTE_THRESHOLD_DIVISOR` or configurable guild setting
|
||
- `MusicManager.voteSkip()` delegates to the new service
|
||
|
||
#### 2. Reduce circular dependencies
|
||
- `TrackScheduler` and `QueueManager` should not need to call back into `MusicManager` for `disconnect()`. Instead, use a callback/listener interface (e.g., `MusicManagerCallback`) that `TrackScheduler` holds, breaking the direct circular reference.
|
||
|
||
#### 3. Fix synchronization
|
||
- Remove `synchronized` from simple delegating methods (`getCurrentlyPlaying()`, `isCurrentlyPlaying()`, `resume()`, `stopTrack()`) — `AudioPlayer` is already thread-safe.
|
||
- Keep `synchronized` only on methods that coordinate multiple state changes (`connect()`, `disconnect()`, `play()`).
|
||
- Make `getQueueSize()` consistent with the chosen strategy.
|
||
|
||
#### 4. Improve error handling
|
||
- `connect()`: Replace `RuntimeException` with `IllegalArgumentException("audioChannel must not be null")`
|
||
- `baseVolume`: Add try-catch with a sensible default (e.g., 50) and a warning log.
|
||
|
||
#### 5. Extract constants
|
||
- `VOTE_THRESHOLD_DIVISOR = 2.1` (or make configurable)
|
||
- `MIN_MEMBERS_FOR_VOTE = 3`
|
||
- `DEFAULT_VOLUME = 50`
|
||
|
||
### File Structure
|
||
| File | Action |
|
||
|------|--------|
|
||
| `music/MusicManager.java` | Modify — remove vote logic, fix sync, fix error handling |
|
||
| `music/VoteSkipService.java` | **New** — extracted vote-skip logic |
|
||
| `music/MusicManagerCallback.java` | **New** — interface to break circular dependency |
|
||
| `music/TrackScheduler.java` | Modify — use callback interface instead of direct `MusicManager` reference where possible |
|
||
| `music/QueueManager.java` | Modify — remove `getMusicManager()` call chain for disconnect |
|
||
|
||
# Delivery Steps
|
||
|
||
### ✓ Step 1: Extract VoteSkipService and constants
|
||
Vote-skip logic is isolated in a new service class and magic numbers are replaced with named constants.
|
||
|
||
- Create `VoteSkipService.java` in `me.bobert.netneutrality.music`
|
||
- Move the vote-skip logic from `MusicManager.voteSkip()` into `VoteSkipService.voteSkip(CommandContext, QueueManager, AudioChannel)`
|
||
- Define constants: `VOTE_THRESHOLD_DIVISOR`, `MIN_MEMBERS_FOR_VOTE`
|
||
- Update `MusicManager.voteSkip()` to delegate to the new service
|
||
- Add `DEFAULT_VOLUME` constant and wrap `baseVolume` parsing in try-catch with fallback
|
||
|
||
### ✓ Step 2: Fix synchronization and error handling
|
||
Synchronization is applied only where needed and error handling is improved.
|
||
|
||
- Remove `synchronized` from simple delegating methods: `getCurrentlyPlaying()`, `isCurrentlyPlaying()`, `resume()`, `stopTrack()`, `setVolume()`
|
||
- Keep `synchronized` on `connect()`, `disconnect()`, `play()`, `isNotConnectedTo()`
|
||
- Replace `RuntimeException` in `connect()` with `IllegalArgumentException`
|
||
- Ensure `getQueueSize()` follows the same synchronization strategy as other methods
|
||
|
||
### ✓ Step 3: Reduce circular dependencies between MusicManager, TrackScheduler, and QueueManager
|
||
The circular reference chain is broken using a callback interface.
|
||
|
||
- Create `MusicManagerCallback.java` interface with method `void onQueueEmpty()` (or similar) to replace direct `MusicManager.disconnect()` calls from `QueueManager`
|
||
- Have `MusicManager` implement the callback interface
|
||
- Update `TrackScheduler` constructor to accept the callback interface instead of the full `MusicManager` where only disconnect/connection behavior is needed
|
||
- Update `QueueManager.clearQueue()` to use the callback instead of `scheduler.getMusicManager().disconnect()` |