netneutrality/.junie/plans/improve-music-manager.md

sessionId

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