Is this optimal? Working with while loops correctly

Help and Feedback Code Review
1

I was working on a shop frame and it required a while loop to be checked, so I thought of something, when you press the shop button, it becomes visible so I did this:


ShopFrame:GetPropertyChangedSignal("Visible"):Connect(function()
	if ShopFrame.Visible == true then
		print("can see frame")
		while wait(1/60) do
			print("running")
			if Player:FindFirstChild("Inventory") and Player.Inventory:FindFirstChild("Bear Trap") then
				notallowed = true
			else
				notallowed = false
			end

			if Player:FindFirstChild("ToolSave")["Bear Trap"].Value == true and notallowed == true then
				for i,v in pairs(Button:GetChildren()) do
					if v:IsA("UIGradient") then
						v:Destroy()
					end
				end
				local Unavaliable = Gradients.Unavaliable:Clone()
				Unavaliable.Parent = Button
				Button.Text = "MAX: 1"
			end
			if Player:FindFirstChild("ToolSave")["Bear Trap"].Value == false and notallowed == false then
				for i,v in pairs(Button:GetChildren()) do
					if v:IsA("UIGradient") then
						v:Destroy()
					end
				end
				local Avaliable = Gradients.Avaliable:Clone()
				Avaliable.Parent = Button
				Button.Text = Cost.." BOLTS"
			end
			if ShopFrame.Visible ~= true then
				break;
			end
		end
	end
end)

Is this optimal?

2

You have one indent more, try changing it to

if ShopFrame.Visible == false then
    return
end
-- Rest of the code

Using wait(1/60) isn’t overall very good idea, since you’re saying, that you want to wait 16ms for every step, so I assume, you want to check the shop frame every single frame rendered. There are better solutions for this use case, for example RunService.Stepped which is invoked every time game frame is rendered.

Another important thing to mention is, that using FindFirstChild in a repeating loop is very bad experience, this is slow function. Try caching these objects if possible.

Lot of your main conditions in the loop are Value checks on ValueBase objects. These objects do have a Changed signal, use them instead of using frequently repeating loops.

1 Like