Clean up repetitive code

Help and Feedback Code Review
#1

Works, but a lot of the code is repetitive. Basically just a settings menu with music and sfx buttons, that when clicking either on or off will turn the specific sound on or off

local SoundService = game:GetService('SoundService')

local Frame = script.Parent

local Music = Frame:WaitForChild('Music')
local SFX = Frame:WaitForChild('SFX')

Music.On.Activated:Connect(function()
	Music.On.ImageColor3 = Color3.fromRGB(200, 200, 200)
	Music.On.Bar.BackgroundColor3 = Color3.fromRGB(200, 200, 200)
	Music.Off.ImageColor3 = Color3.fromRGB(125, 125, 125)
	Music.Off.Bar.BackgroundColor3 = Color3.fromRGB(125, 125, 125)
	
	local Sound = SoundService:FindFirstChild('Music')
	if not Sound then return end
	
	Sound.Volume = 0.5
end)

Music.Off.Activated:Connect(function()
	Music.Off.ImageColor3 = Color3.fromRGB(200, 200, 200)
	Music.Off.Bar.BackgroundColor3 = Color3.fromRGB(200, 200, 200)
	Music.On.ImageColor3 = Color3.fromRGB(125, 125, 125)
	Music.On.Bar.BackgroundColor3 = Color3.fromRGB(125, 125, 125)
	
	local Sound = SoundService:FindFirstChild('Music')
	if not Sound then return end
	
	Sound.Volume = 0
end)

SFX.On.Activated:Connect(function()
	SFX.On.ImageColor3 = Color3.fromRGB(200, 200, 200)
	SFX.On.Bar.BackgroundColor3 = Color3.fromRGB(200, 200, 200)
	SFX.Off.ImageColor3 = Color3.fromRGB(125, 125, 125)
	SFX.Off.Bar.BackgroundColor3 = Color3.fromRGB(125, 125, 125)
	
	local Sound = SoundService:FindFirstChild('SFX')
	if not Sound then return end
	
	Sound.Volume = 0.5
end)

SFX.Off.Activated:Connect(function()
	SFX.Off.ImageColor3 = Color3.fromRGB(200, 200, 200)
	SFX.Off.Bar.BackgroundColor3 = Color3.fromRGB(200, 200, 200)
	SFX.On.ImageColor3 = Color3.fromRGB(125, 125, 125)
	SFX.On.Bar.BackgroundColor3 = Color3.fromRGB(125, 125, 125)
	
	local Sound = SoundService:FindFirstChild('SFX')
	if not Sound then return end
	
	Sound.Volume = 0
end)
2 Likes
#2

Put all the repetitive code in a function. It seems like you’re changing the volume and color of SFX/Music GUIs, so here’s how your function should work:

  • There should be 2 parameters: on and soundName
  • on is a boolean that will determine the color of the GUIs and volume of the SoundEffect
  • soundName is a string that will determine the GUI and sound to be used in the code

Then you use a for loop to iterate through the children of Frame, assuming SFX and Music are the only children. Connect a function to the GUI’s On and Off buttons and call the function with the repeated code with the appropriate argument values

Let me know if you’re still having trouble, I’ll write the code out

2 Likes
#3

I usually go for a function in a function for these situations.

local SoundService = game:GetService("SoundService")

local Frame = script.Parent
local Music = Frame:WaitForChild("Music")
local SFX = Frame:WaitForChild("SFX")

local Colors = {
    On = Color3.fromRGB(200, 200, 200);
    Off = Color3.fromRGB(125, 125, 125);
}

local function BindToggle(button, otherButton)
    if not button:IsA("GuiButton") then
        return
    end

    button.Activated:Connect(function()
        button.ImageColor3 = Colors.On
        button.Bar.BackgroundColor3 = Colors.On
        otherButton.ImageColor3 = Colors.Off
        otherButton.Bar.BackgroundColor3 = Colors.Off

        local Sound = SoundService:FindFirstChild(button.Parent.Name)
        if not Sound then
            warn("Couldn't find sound instance.")
            return
        end

        Sound.Volume = button.Name == "On" and 0.5 or 0
    end
end

BindToggle(Music.On, Music.Off); BindToggle(Music.Off, Music.On)
BindToggle(SFX.On, SFX.Off); BindToggle(SFX.Off, SFX.On)

I got all time on my side.

1 Like
#4 
local function toggleSound(on, soundName)
	local Sound = SoundService:FindFirstChild(soundName)
	if not Sound then return end
	
	Sound.Volume = 0.5
end

for _, v in pairs(Frame) do
	if v.Name == 'Music' or v.Name == 'SFX' then
		v.On.Activated:Connect(function()
			toggleSound(true, v.Name)
		end)
		
		v.Off.Activated:Connect(function()
			toggleSound(false, v.Name)
		end)
	end
end

This is what I got, but not sure how to change the colors of things based on just a true or false statement

#5

You’ll have to make a dictionary of the colors:

local Colors = {
    On = Color3.fromRGB(200, 200, 200);
    Off = Color3.fromRGB(125, 125, 125);
}

Then you can use if statements or the shortcut for assigning values using a bool as such:

local gui = Frame:FindFirstChild(soundName)
gui.On.ImageColor3 = on and Colors.On or Colors.Off
-- and so on

var = bool and ifTrue or ifFalse
if bool is truthy (not false and not nil), var will be assigned the value ifTrue, else it will be ifFalse

1 Like
#6 
local function toggleSound(on, soundName)
	Music.On.ImageColor3 = on and Colors.On or Colors.Off
	Music.Off.ImageColor3 = not on and Colors.Off or Colors.On
	
	SFX.On.ImageColor3 = on and Colors.On or Colors.Off
	SFX.Off.ImageColor3 = not on and Colors.Off or Colors.On
end

This didn’t seem to work

#7

I’m on my PC now.

You’re using toggleSound whenever the music/sfx on/off button is pressed. So you would need to only change the color of the button pressed, I’m not sure why you’re changing both music and sfx on and off button colors.

Your logic for setting the color for the Off buttons is also wrong.

Music.Off.ImageColor3 = not on and Colors.Off or Colors.On

When on = true, not on would evaluate to false and hence it would assign the color to Colors.On instead of Colors.Off, which is what you want.

Here’s how you could do it:

local function toggleSound(on, soundName)
    local gui = Frame:FindFirstChild(soundName)
    gui.On.ImageColor3 = on and Colors.On or Colors.Off
    gui.Off.ImageColor3 = on and Colors.Off or Colors.On
    
    local Sound = SoundService:FindFirstChild(soundName)
    if not Sound then return end

    Sound.Volume = on and 0.5 or 0
end