---
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()`