Music Request System Feedback/Improvement

Hello, recently I’ve been working on improving my Music System.

It also has been having a little bit of issues of skipping songs to find one, becuase I added this new count variable to help me skip songs that aren’t valid.

Any idea on how I can improve my code?

--[ SERVICES ]--

local Players = game:GetService("Players") -- The Player service.
local rep = game:GetService("ReplicatedStorage") -- The ReplicatedStorage service.
local MarketplaceService = game:GetService("MarketplaceService") -- The Marketplace service.


local CompletedBindable ="BindableEvent")
CompletedBindable.Name = "CompletedBindable"
CompletedBindable.Parent = game:GetService("ServerStorage")

local events = rep.EVENTS.MusicEvents -- This variable is the folder in which the remote events/functions are stored in.

local queue = {}
local songs = {
} -- This table is where all the songs will be stored.

local currentSong = "No song playing"

local sound = game.Workspace:FindFirstChild("GlobalSound")


events.GetPlayingSong.OnServerInvoke = function()
	return currentSong -- When the GetPlayingSong remote function gets invoked, we will return the currently playing song name back to the client who invoked it.


events.RequestSong.OnServerEvent:Connect(function(player, id)
	print("REQUEST SONG WAS FIRED : " .. id)
	if id then
		local success, data = pcall(MarketplaceService.GetProductInfo, MarketplaceService, id)
		if success and data and data.AssetTypeId == 3 then
			table.insert(queue, id) -- inserting Id into queue table
			events.RefreshQueue:FireAllClients(queue) -- firing refresh queue for later


while true do
	if #songs > 0 then -- We first check if there are any songs implemented into the system. If so, we proceed.
		sound.TimePosition = 0 -- We set the TimePosition property of our sound object to 0. This is done so when a new song starts playing, it doesn't continue from the TimePosition of when the previous song ended at.
		local selectedSong -- We create our selectedSong variable. It will be properly declared in the following code.
		if #queue > 0 then -- We check if there are any requested songs. If so, we select the first song that was requested.
			selectedSong = queue[1] -- Sets the selected song variable as the requested song.
			table.remove(queue, 1) -- Removes the selected song from the queue table.
			events.RefreshQueue:FireAllClients(queue) -- We fire all clients with the RefreshQueue remote event to refresh the queue as it was modified.
			selectedSong = songs[math.random(1, #songs)] -- If there are no requested songs, then we select a random song from our songs table.
		sound.SoundId = "rbxassetid://"..tostring(selectedSong) -- We set the SoundId property of the sound object to our selected song's ID.
		local count = 0
			count = count + .1
		until sound.IsLoaded or count >= 4

		if sound.IsLoaded then
			--local asset = game:GetService("MarketplaceService"):GetProductInfo(selectedSong) 
			--local asset_name = asset.Name
			--if asset_name:lower():find("[ Content Deleted ]")then
			--	table.remove(songs,selectedSong)
				sound.PlaybackSpeed = 1 -- We set the PlaybackSpeed property of the sound to the Pitch index of our selected song. If there is no pitch index, then we set it to 1.
				sound:Play() -- We begin playing the song.
				events.NewSong:FireAllClients(selectedSong) -- We fire the NewSong event to all clients to indicate a new song has started playing.
				currentSong = selectedSong -- We set the currentSong variable to the name of the selected song.
	else -- if no songs playing we break out of the loop at 98 and change the text of the sign

sound.Ended:Connect(function() CompletedBindable:Fire() end)
sound.Stopped:Connect(function() CompletedBindable:Fire() end)


I had to comment out the local asset part, because it seems to be causing a little problem. Any idea what I can do to improve this script?

1 Like
if asset_name:lower():find("[ Content Deleted ]") then

You’re uncapitalising the string with “:lower()” and then attempting to find a string which contains capitals within it. You would need to do one of the following instead.

if asset_name:find("[ Content Deleted ]") then

if asset_name:lower():find("[ content deleted ]") then

Would that make sense to do this? Also with table.remove(songs,selectedSong)
then firing bindable event again? How does that look.

1 Like
if asset_name:find("[ Content Deleted ]") then
	table.remove(songs, table.find(songs, selectedSong))

table.remove() requires the index of the item/entry of which you wish to remove.