Skip to content

[Code health] MyMusicService buildXItems functions return MutableList unnecessarily #249

Description

@kimptoc

Summary

All 25+ build...Items() functions in shared/src/main/java/com/example/mymediaplayer/shared/MyMusicService.kt declare MutableList<MediaItem> return types, but no caller mutates the result. Every call site either reads the list (sendResult, .size, iteration) or passes it through unchanged. The MutableList contract is therefore stronger than the actual API needs, and several call sites end up doing redundant work to honour it (e.g. .toMutableList() after a .map { } that already produced a List).

Affected functions

buildMediaItems
buildMediaItemsForId
buildMediaItemsForPrefix
buildMediaItemsForSmartPlaylist
buildMediaItemsForPlaylist
buildMediaItemsForAlbum
buildMediaItemsForGenre
buildMediaItemsForGenreSongLetter
buildMediaItemsForArtist
buildMediaItemsForArtistLetter
buildMediaItemsForDecade
buildMediaItemsForDecadeSongLetter
buildMediaItemsForGenreLetter
buildMediaItemsForSongLetter
buildRootItems
buildSongsItems
buildSongsAllItems
buildPlaylistsItems
buildAlbumsItems
buildGenresItems
buildArtistsItems
buildDecadesItems
buildSearchItems
buildPlayAllShuffleItems
buildHomeItems

Why it's worth fixing

  • No caller actually mutates the returned list. Confirmed by surveying call sites: every browse path goes buildMediaItems(...) → result.sendResult(...) (which accepts List), .size, or iteration. The mutability is part of the type signature only.
  • Eliminates a real allocation in at least one path. buildPlaylistsItems() already had to call .toMutableList() on its .map { }.toMutableList() chain just to satisfy the declared return type — one copy of ~10 entries per AA playlists-root browse. PR Remove [Play All] / [Shuffle] from AA Playlists root (#245) #248 included this as part of the cleanup but did not change the return-type chain. See the PR thread for the original kilo-code-bot nit and the rationale for deferring the fix.
  • Tightens the API contract. MediaBrowserServiceCompat.Result.sendResult(List<MediaItem>) only requires List. Returning List<MediaItem> from the browse-builder chain matches the actual consumer contract and removes a false signal to future readers that "the caller might mutate this."

Proposed change

Change the return type of every build...Items() function listed above from MutableList<MediaItem> to List<MediaItem>. Mechanical refactor — Kotlin's type inference handles most of the call sites without further changes.

Specific follow-ups in the same PR:

  • Drop the .toMutableList() at the end of buildPlaylistsItems() (line ~1019).
  • Wherever a builder accumulates with val items = mutableListOf<MediaItem>(); items.add(...); items += ...; return items — that's fine internally, return a List<MediaItem> from mutableListOf's upcast; the internal local var can stay MutableList. The only required change is the external return type.
  • Any nullable variant (MutableList<MediaItem>? on buildMediaItemsForPrefix) becomes List<MediaItem>?.

Out of scope

  • Refactoring how browse items are accumulated internally (the MutableList local-variable pattern is fine).
  • Changes to MediaBrowserServiceCompat's own API surface.
  • Performance work beyond the consequential allocation removal — this is a code-health issue, not a perf issue.

Acceptance criteria

  • All build...Items() functions return List<MediaItem> (or List<MediaItem>? for the nullable variant).
  • Existing tests still pass (./gradlew :shared:testDebugUnitTest).
  • ./gradlew :shared:assembleDebug :mobile:assembleDebug still succeeds.
  • grep -nE "MutableList<MediaItem>" returns only internal local-variable declarations, not function return types.

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions