Need feedback on my script

Help and Feedback Code Review
1 
local Players = game:GetService("Players")
local Player = Players.LocalPlayer
local PlayerGui = Player:WaitForChild("PlayerGui")

local ShopUI = PlayerGui:WaitForChild("ShopUI")
local SideUI = PlayerGui:WaitForChild("SideUI")
local UpgradeUI = PlayerGui:WaitForChild("UpgradeUI")

local MainFrame = ShopUI:WaitForChild("Frame")
local Categories = MainFrame.Categories.ButtonBar
local ItemFrame = MainFrame.Items.ItemsFrame.ScrollingFrame
local Passes = MainFrame.Passes.GamepassFrame
local ViewFrames = MainFrame.ViewFrames

local MedKitLevel = 10
local CrowbarLevel = 25
local BackpackLevel = 50
local KeyDetectorLevel = 100

local function unViewLockedFrame(Frame)
	Frame.LockedFrame.Visible = false
	Frame.BuyFrame.Visible = true
end

for _, ViewFrame in ipairs(ItemFrame:GetChildren()) do
	
	if ViewFrame:IsA("ImageLabel") then
		
	Player.leaderstats.Level.Changed:Connect(function()	
		
	if ViewFrame.Name == "MedKit" then
		if Player.leaderstats.Level.Value >= MedKitLevel then
			unViewLockedFrame(ViewFrame)
		end
			
	elseif ViewFrame.Name == "Crowbar" then
		if Player.leaderstats.Level.Value >= CrowbarLevel then
			    unViewLockedFrame(ViewFrame)
			end
			
	elseif ViewFrame.Name == "Backpack" then
		if Player.leaderstats.Level.Value >= BackpackLevel then
			   unViewLockedFrame(ViewFrame)
		   end
			
	elseif ViewFrame.Name == "KeyDetector" then
		if Player.leaderstats.Level.Value >= KeyDetectorLevel then
			       unViewLockedFrame(ViewFrame)
			    end
			end
	    end)
    end
end

return 1

The reason I’m using return 1 is because that it’s a module script called by a local script ( for organization ).

And I’m pretty certain that my script isn’t that ‘efficient’ at all.

2

You should use one Changed event instead of 4, considering they’re all listening for the same event. Loop inside the Changed event rater than outside.

3 Likes
3

Yes, let me update the script I did that already.

4

You still have a new Changed event created for each ImageLabel you iterate through, though. My recommendation was to have the GetChildren loop within the function you connect to on Level.Changed.

3 Likes
5

Alright. Now that makes sense, thanks.

local Players = game:GetService("Players")
local Player = Players.LocalPlayer
local PlayerGui = Player:WaitForChild("PlayerGui")

local ShopUI = PlayerGui:WaitForChild("ShopUI")
local SideUI = PlayerGui:WaitForChild("SideUI")
local UpgradeUI = PlayerGui:WaitForChild("UpgradeUI")

local MainFrame = ShopUI:WaitForChild("Frame")
local Categories = MainFrame.Categories.ButtonBar
local ItemFrame = MainFrame.Items.ItemsFrame.ScrollingFrame
local Passes = MainFrame.Passes.GamepassFrame
local ViewFrames = MainFrame.ViewFrames

local MedKitLevel = 10
local CrowbarLevel = 25
local BackpackLevel = 50
local KeyDetectorLevel = 100

local function unViewLockedFrame(Frame)
	Frame.LockedFrame.Visible = false
	Frame.BuyFrame.Visible = true
end

Player.leaderstats.Level.Changed:Connect(function()	
for _, ViewFrame in ipairs(ItemFrame:GetChildren()) do
	
	if ViewFrame:IsA("ImageLabel") then
		
	if ViewFrame.Name == "MedKit" then
		if Player.leaderstats.Level.Value >= MedKitLevel then
			unViewLockedFrame(ViewFrame)
		end
			
	elseif ViewFrame.Name == "Crowbar" then
		if Player.leaderstats.Level.Value >= CrowbarLevel then
			    unViewLockedFrame(ViewFrame)
			end
			
	elseif ViewFrame.Name == "Backpack" then
		if Player.leaderstats.Level.Value >= BackpackLevel then
			   unViewLockedFrame(ViewFrame)
		   end
			
	elseif ViewFrame.Name == "KeyDetector" then
		if Player.leaderstats.Level.Value >= KeyDetectorLevel then
			       unViewLockedFrame(ViewFrame)
			    end
			end
        end
    end
end)

return 1
2 Likes