Is there anything wrong or bad practice with this 3D button function?

Help and Feedback Code Review
#1

I have a script that simulates pressing a button as if it was in 3d(no idea how else to word it). The thing that I usually do a lot in my scripts is condense GUI’s instances into a table then run a for loop that attaches events to each one of these buttons. I thought it was a pretty good idea but I’m not sure anymore.

I have a lot of these close buttons you see throughout my game inside these UI’s. Is there anything wrong with this code? Like for example… memory leaks? There’s a video at the button to help you.

local Hud = {}
-- Other functions

-- Other functions

-- Other functions
function Hud:TweenCloseButtons()
	local HoldInfo = TweenInfo.new(.05,Enum.EasingStyle.Linear,Enum.EasingDirection.Out)
	local ReleaseInfo = TweenInfo.new(.25,Enum.EasingStyle.Linear,Enum.EasingDirection.Out)
	
	local SelectHouse = script.Parent.SelectHouse
	
	local CloseButtons = {
		script.Parent.Buildings.Background.CloseButton,
		script.Parent.Shop.CloseButton,
		script.Parent.Inventory.CloseButton
	}
	
	for i,v in pairs(CloseButtons) do
		local Hold = nil
		local Release = nil
		local OriginalPosition = nil
		v.UserInput.MouseEnter:Connect(function()
			if OriginalPosition then -- Hacky method to fix a bug.
				v.UserInput.Position = OriginalPosition 
			end
			v.UserInput.BackgroundColor3 = Color3.fromRGB(244, 22, 33)
			OriginalPosition = v.UserInput.Position
			Hold = TweenService:Create(v.UserInput,HoldInfo,{Position = v.depth.Position})
			Release = TweenService:Create(v.UserInput,ReleaseInfo,{Position = OriginalPosition})
		end)
		v.UserInput.MouseLeave:Connect(function()
			v.UserInput.BackgroundColor3 = Color3.fromRGB(220, 20, 30)
			Release:Play()
		end)
		v.UserInput.MouseButton1Down:Connect(function()
			Hold:Play()
		end)
		v.UserInput.MouseButton1Up:Connect(function()
			if i == 1 then
				v.Parent.Parent.Visible = false
				v.Parent.Parent.Position = UDim2.new(0.675,-90,0.5,0)
			elseif i == 2 then
				v.Parent.Visible = false
				v.Parent.Position = UDim2.new(0.535,10,0.561,0)
			elseif i == 3 then
				v.Parent.Visible = false
				SelectHouse.Visible = false
				v.Parent.Position = UDim2.new(0.525,0,0.55,0)
			end
			Hud.TweenHudRight()
		end)
	end
end

-- Other functions

return Hud
#2

Since your function isn’t relating to an object, you should be using function module.name() syntax instead of function module:name() syntax.

Some code is being put in the loop when its behavior is better expressed as independent statements. Take the following:

v.UserInput.MouseButton1Up:Connect(function()
	if i == 1 then
		v.Parent.Parent.Visible = false
		v.Parent.Parent.Position = UDim2.new(0.675,-90,0.5,0)
	elseif i == 2 then
		v.Parent.Visible = false
		v.Parent.Position = UDim2.new(0.535,10,0.561,0)
	elseif i == 3 then
		v.Parent.Visible = false
		SelectHouse.Visible = false
		v.Parent.Position = UDim2.new(0.525,0,0.55,0)
	end
	Hud.TweenHudRight()
end)

An alternative could be, outside the loop:

CloseButtons[1].UserInput.MouseButton1Up:Connect(function()
    v.Parent.Parent.Visible = false
    v.Parent.Parent.Position = UDim2.new(0.675,-90,0.5,0)
    Hud.TweenHudRight()
end)
-- then do the same with the different code of 2 and 3
5 Likes