Using 1 event for multiple triggers

Help and Feedback Scripting Support
1

So im scripting a gui catalog and I put all this in one event and just fire each toggle instead of making an event for each different toggle. My question is are there any better ways to script this / will this script cause any game issues (Im still learning how to script so this might seem like a obvious question)

game:GetService("ReplicatedStorage").Events.AccessoryAdded.BodyCatalog:FireServer("RemoveShirt")

game:GetService("ReplicatedStorage").Events.AccessoryAdded.BodyCatalog.OnServerEvent:Connect(function(player, toggle)
	if not player.Character then return end
	if toggle == "RemoveShirt" then
		--//Code
	end
	if toggle == "RemoveTShirt" then
		--//Code
	end
	if toggle == "RemoveAccessories" then
       --//Code
	end
	if toggle == "RemoveHair" then
		--//Code
	end
	if toggle == "RemoveHats" then
		--//Code
	end
end)
2

Looks great to me. Also nice and organized.
Could add returns but, should be fine anyways.

  if toggle == "RemoveShirt" then
		--//Code
     return
  end

This is pretty straight forward and still easy to change and edit. Could go with elseif statements. For ME that has a count limit you’re just over… the returns are just as effective. No need to over complicate things.

1 Like
3

its better to use elseif instead of a bunch of if’s
also if you want a more organized approach you could do something like this:

local Functions = {
	["RemoveShirt"] = function()
		--// code
	end, 

	["RemoveTShirt"] = function()
		--// code
	end, 
}


game:GetService("ReplicatedStorage").Events.AccessoryAdded.BodyCatalog.OnServerEvent:Connect(function(player, toggle)
	if not player.Character then return end
	if not Functions[toggle] then return end 

	Functions[toggle]()
end)
3 Likes
4

That’s not optimized for this case… same as variables, each function are allocating more memory usage, so adding different functions for each kind of character item isn’t the best idea, especially when each of them are doing the exact same thing which is to destroy a few instances.

I think it could be better to directly send the instance(s) as argument additionnaly of the toggle mode, so you can reduce it to only 3 elseif.

--Add a new item
RemoteEvent:FireServer("Add", {})

--Remove an existing item
RemoteEvent:FireServer("Remove", {})

--Remove an existing item and add the new item
RemoteEvent:FireServer("Switch", {}, {})

-- The table {} contain the instances you want to add/remove
-- It is just in case you want to delete/add multiple things at same time.
-- To avoid excessively firing the event.

local ItemList = {}

local Item1 = AccessoriesStorage:FindFirstChild("AccessoryName")
local Item2 = AccessoriesStorage:FindFirstChild("AccessoryName")

table.insert(ItemList, Item1)
table.insert(ItemList, Item2)

RemoteEvent:FireServer("Add",  ItemList)

--------------
local ItemList = {}

local Item1 = Character:FindFirstChild("AccessoryName")
local Item2 = Character:FindFirstChild("AccessoryName")

table.insert(ItemList, Item1)
table.insert(ItemList, Item2)

RemoteEvent:FireServer("Remove",  ItemList)

--------------
local ItemList = {}

for _, Child in Character:GetChildren() do
    if Child:IsA("Accessory") then
        table.insert(ItemList, Child)
    end
end

RemoteEvent:FireServer("Remove",  ItemList)

Server Side

RemoteEvent.OnServerEvent:Connect(function(Player, Mode, Table1, Table2)
    if Mode == "Add" then
        for _, Child in Table1 do
            Child:Clone().Parent = Player.Character
        end
    elseif Mode == "Remove" then
        for _, Child in Table1 do
            Child:Destroy()
        end
    elseif Mode == "Switch" then
        for _, Child in Table1 do
            Child:Destroy()
        end
        for _, Child in Table2 do
            Child:Clone().Parent = Player.Character
        end
    end
end)