Review this background music player

Help and Feedback Code Review
#1

Hello, i created a simple script that playbacks sound depending in the level which the player is, plus some other features, i want to know if i can optimize it more, thanks.
Code here:

--Setup
Music = script.Sound
IdMap = {}
IdMap[0] = script.LoadSong
IdMap[1] = script.Song1
IdMap[2] = script.Song2
IdMap[3] = script.Song3
IdMap[4] = script.Song4
IdMap[5] = script.Song5
IdMap[6] = script.Song6
IdMap[7] = script.Song7
IdMap[8] = script.Song8
IdMap[9] = script.Song9
IdMap[10] = script.Song10
IdMap[11] = script.Song11
--Select music for level
function OnLevelPass()
	local Level = game.Players.LocalPlayer:WaitForChild("CheckPointNumber")
	local ID = nil
	if Level.Value < 12 then
		 ID = IdMap[Level.Value].Value
	else --Handle Levels higher than 11
		 ID = IdMap[Level.Value - 11].Value 
	end
	if Level.Value == 17 then --Select different music for end area
		ID = IdMap[0].Value
	end
	ChangeSound(ID)
end
--Handle Game Loading
function OnLoaded()
	OnLevelPass() --Resume Level Music
end
function OnCredits()
	if game.Players.LocalPlayer.Credits.Value == true then
		local ID = IdMap[8].Value --Start credits sound
		ChangeSound(ID)
	end
	if game.Players.LocalPlayer.Credits.Value == false then
		OnLevelPass() --Resume Level Music
	end
end
--Play the Music
function ChangeSound(ID)
	Music:Stop()
	Music.SoundId = "rbxassetid://"..ID
	Music:Play()
end
--Start Loading Music
local ID = IdMap[0].Value
ChangeSound(ID)
--Binding
game.Players.LocalPlayer:WaitForChild("Load"):GetPropertyChangedSignal("Value"):connect(OnLoaded)
game.Players.LocalPlayer:WaitForChild("Credits"):GetPropertyChangedSignal("Value"):connect(OnCredits)
game.Players.LocalPlayer:WaitForChild("CheckPointNumber"):GetPropertyChangedSignal("Value"):connect(OnLevelPass)

Feel free to use this code yourself!

6 Likes
#2

For the IdMap table, use a for loop at the start so you don’t have to manually add it in every time.

for i_,v in pairs(script:GetChildren()) do
    if v:IsA("Sound") then
        table.insert(IdMap,v);
    end
end

Other than that, looks good.

1 Like
#3

I’m going to nickpit a bit.
Use GetService instead of game.Players incase you end up changing the services name.

Then I think you’re probably good to go.

#4

Don’t use game.Players.LocalPlayer so much. It’s better to just make that a local variable: local player = game:GetService(“Players”).LocalPlayer

#5

Which in itself is bad practice and should never be done.

1 Like
#6

I would use this loop because it is really efficient.

local children = script:GetChildren()
for i = 0, #children do
   if children[i]:IsA('Sound') then
      table.insert(IdMap, children[i]);
   end
end

I would also personally change this function, but it is not needed

function OnLevelPass()
	local Level = game.Players.LocalPlayer:WaitForChild("CheckPointNumber")
	local ID = Level.Value < 12 and IdMap[Level.Value].Value or Level.Value == 17 and IdMap[0].Value or ID = IdMap[Level.Value - 11].Value
	ChangeSound(ID)
end
1 Like
#7

There is a lot of duplocation of code so I would first create some variables for these.

You should also be using local functions ie local function ChangeSound() or anonymous functions which help reduce namespace bloating.

Lastly if Load, Credits and CheckPointNumber are value objects then you should be using Changed over GetPropertyChangedSignal.

Other
I would also use waitforchild on script.LoadSong ect to be safe.

You compare a boolean with a boolean which is pointless in your code.

game.Players.LocalPlayer.Credits.Value == true

The result would be one of the following, true == true (this is true), false == true (this is false).
In all cases you would just use
if game.Players.LocalPlayer.Credits.Value then

Overall I would look at simplifying this code where possible.

#8

I wouldn’t recommend storing your “Songs” inside your scripts, I think you should pick a different container based on what permissions you want other scripts and services to have on it. Personally, i would put them in ReplicatedStorage.

#9

The sounds are not stored, just the game ids, also this new 50 character limit sucks.

#10

Here it is, a updated version of the code using all of your sugestions.

--Setup
local player = game:GetService(“Players”).LocalPlayer --New variable to avoid repeating.
local Music = script.Sound
local IdMap = {}
--NEW SHINY LOOP
local children = script:GetChildren()
for i = 0, #children do
   if children[i]:IsA('IntValue') then
      table.insert(IdMap, children[i]);
   end
end
--Simplified function (handles stage number)
local function OnLevelPass()
	local Level = player:WaitForChild("CheckPointNumber")
	local ID = Level.Value < 12 and IdMap[Level.Value].Value or Level.Value == 17 and IdMap[0].Value or ID = IdMap[Level.Value - 11].Value
	ChangeSound(ID)
end
end
--Handle Game Loading
local function OnLoaded()
	OnLevelPass() --Resume Level Music
end
local function OnCredits()
	if player.Credits.Value then
		local ID = IdMap[8].Value --Start credits sound
		ChangeSound(ID)
	else
		OnLevelPass() --Resume Level Music
	end
end
--Play the Music
function ChangeSound(ID)
	Music:Stop()
	Music.SoundId = "rbxassetid://"..ID
	Music:Play()
end
--Start Loading Music
local ID = IdMap[0].Value
ChangeSound(ID)
--Binding (Changed up)
player:WaitForChild("Load"):Changed():connect(OnLoaded)
player:WaitForChild("Credits"):Changed():connect(OnCredits)
player:WaitForChild("CheckPointNumber"):Changed():connect(OnLevelPass)

Note that i didn’t test this code yet, so there may be some bugs.

2 Likes
#11

I noticed that the loop is broken, anyone as an idea of why is that occuring

#12

If your code is no longer working, it does not belong in Code Review any longer. A branch post should be created in Scripting Support regarding the issue you’re experiencing. Once that issue is resolved, you can come back to this thread and update it, or start a new thread. If you do start a new thread, please ensure you mark any one of these posts that best helped you improve your code as the solution.

#13

No, it’s not my code that’s broken, it’s Thezo’s loop that’s broken

#14

Don’t use his loop then. Code Review is strictly for improvement of working code. Anything broken should be fixed or not used. If it’s affected your code, either backpedal to a working version or create a new thread in Scripting Support (only after you’ve diagnosed the code yourself).