Is this a bad coding practice?

[LOCK3DOOR]

Hello, I have recently been making a game where there is a bunch of tools that give you a different gain amount based on that tool, and I was wondering if this is a bad coding practice mainly because it is highlighted with a warning. Particularly the parts with an arrow pointing to them.

local Player = game.Players.LocalPlayer
local Character = script.Parent

Character.ChildAdded:Connect(function(child)
	if child:IsA("Tool") and child:GetAttribute("Gain") then
	-->> ActivatedConnection = child.Activated:Connect(function()
			game.ReplicatedStorage.RemoteEvents.Event:FireServer()
		end)
		child.Unequipped:Connect(function()
		-->> ActivatedConnection:Disconnect()
		end)
	end
end)

Basically, I am checking when a tool is equipped, and if it has a gain value. But I was wondering if there is a better way of doing this.

[kingerman88]

The warning is just indicating that you are declaring a global (which means that it could be accessed outside of the scope of the Character.ChildAdded function

Adding a local removes the warning and is also generally good practice (unless you need to access the ActivateConnection variable outside of the scope of the function, but in that case you should also define a local variable outside of the scope to make things more clear)

local Player = game.Players.LocalPlayer
local Character = script.Parent

Character.ChildAdded:Connect(function(child)
	if child:IsA("Tool") and child:GetAttribute("Gain") then
	    local ActivatedConnection = child.Activated:Connect(function()
			game.ReplicatedStorage.RemoteEvents.Event:FireServer()
		end)
		child.Unequipped:Connect(function()
		    ActivatedConnection:Disconnect()
		end)
	end
end)

[LOCK3DOOR]

I tried doing a local on the connection but then it does the warning line on the Disconnect() part.

[kingerman88]

Unless I copied the script incorrectly / missed something, I don’t see any warnings on my end

(Also something that I noticed is that the child.Unequipped function will probably result in a memory leak, since every time the tool is equipped it will create another unequipped connection)

[LOCK3DOOR]

Nvm. I thought it would be underlined but I guess not. Is there a solution for the potential memory leak issue?

[kingerman88]

Add another variable for the unequip connection.
Note that you define the variable first so that you can access it within the function)

local Player = game.Players.LocalPlayer
local Character = script.Parent

Character.ChildAdded:Connect(function(child)
	if child:IsA("Tool") and child:GetAttribute("Gain") then
		local ActivatedConnection = child.Activated:Connect(function()
			game.ReplicatedStorage.RemoteEvents.Event:FireServer()
		end)
		local UnequipConnection
		UnequipConnection = child.Unequipped:Connect(function()
			ActivatedConnection:Disconnect()
			UnequipConnection:Disconnect()
		end)
	end
end)

[LOCK3DOOR]

Ehh. That is getting kind of complicated for a simple idea at mind. Maybe I could use a global connection then detect when the character unequips the tool?

E.g:

Character.ChildAdded:Connect(function(child)
	if child:IsA("Tool") and child:GetAttribute("Gain") then
		ActivatedConnection = child.Activated:Connect(function()
			game.ReplicatedStorage.RemoteEvents.Event:FireServer()
		end)
	end
end)

Character.ChildRemoved:Connect(function(child)
	if child:IsA("Tool") and child:GetAttribute("Gain") then

			ActivatedConnection:Disconnect()
	
    end
end)

[kingerman88]

That also should work, you can use that if you want

[TestAccount563344]

very simple fix (i havent tested this, but this is what i commonly do)

local Player = game.Players.LocalPlayer
local Character = script.Parent

Character.ChildAdded:Connect(function(child)
	if child:IsA("Tool") and child:GetAttribute("Gain") then
	    local ActivatedConnection; ActivatedConnection = child.Activated:Connect(function() --this is the only thing i changed
			game.ReplicatedStorage.RemoteEvents.Event:FireServer()
		end)
		child.Unequipped:Connect(function()
		    ActivatedConnection:Disconnect()
		end)
	end
end)

setting it as a local variable and assigning the function at the same time has given me the warning before, so this is my fix.

[LOCK3DOOR]

Thanks for the help! First actual solution I have gotten in a while.

[TimeVector]

Keep in mind that :Once() is also an option. Same as :Connect(), but it will automatically disconnect after firing the event once.