How could I optimize this code?

[El_BananitoPocho]

REMOVED

It’s just a big for loop. It basically tells wich sound to play when buttons in a GUI are clicked, but I feel like there’s a more optimized way of doing this? Could you guys help me? All ideas are appreciated, thanks in advance.

[Amritss]

This code is a big victim of memory leaks, since you’re connecting an event to a function every time the “MouseEnter” event is fired.

Memory leaks can lead to crashes and what not so just put these “MapButton” UI stuff in a folder, loop through the children in a seperate script and connect an event to a function for each one which will play the sound when clicked. Instead of doing this.

[El_BananitoPocho]

Could you show me some sample code for this? Also, the different buttons are meant to be in different UI Frames so I don’t think I could group them in a Folder.

[Amritss]

You can use collectionservice and give each of them a tag:

for _, button in Cs:GetTagged("Tag") do
button.Activated:Connect(function()
if button.Name == "ButtonName" then
ButtonSound:Play()
end
end
end

[El_BananitoPocho]

Made some changes. How’s it looking now?

REMOVED

[Amritss]

Um, still a memory leak.

Just do this

for _, button in CollectionService:GetTagged("GuiButton") do
if button.Name == "YourName" then
-- Set up event here
elseif button.Name == "YourName" then
-- Set up event
-- Keep making elseif statements

What your current code is doing is connecting the same event to a function everytime, it’s kinda hard to explain. But let’s say the button value that you’re currently on is NOT named MapButtonOpen, you’re still connecting the function to that event anyway.

[0786ideal (ideal)]

you can replace all of those ifs by one if and table, then in table there will be specific functions that will fire when needed

[El_BananitoPocho]

Thanks for your answers, guys, I’ll implement this into my project.

[https_KingPie]

One thing I’d recommend from a design philosophy POV is adding an initialization function that handles things like your connections.

It can be as simple as:

local Initialized = false

local function Initialize()
    if Initialized then
        return
    end

    Initialized = true

    --// Code below
end

And whenever you open your UI/run your code, just call Initialize(). It’s a simple way of ensuring that your one-time use code (whether that is binding things to buttons or getting more particularly with hover effects, code to properly scale your UI elements, etc.) only runs one time.

Additionally, if you want to save yourself work on cleanup code that manages disconnecting events, you can always just disable the ScreenGui.ResetOnSpawn Property. By default this is enabled, but from my experience most UIs don’t really need to be destroyed when the player dies. Destroying them can lead to weird edge cases too. This way, paired with an initialization function, you can just functionally set-up your UI once.

[El_BananitoPocho]

I see, thanks for showing me this. And yes, I always disable ResetOnSpawn now.