How can I make my code more efficent

Help and Feedback Code Review
#1

I have this piece of code that basically just looks at what your mouse has hovered over and sees if it should display a billboard GUI over it. I was wondering if there is a more efficient and faster way to check if it should display the GUI then the way I have right now.

UIS.InputChanged:Connect(function(input)
	if mouse.Target then
		if mouse.Target:FindFirstChild("Pickable") then
			local item = mouse.Target
			EtoPickup.Text = "E - Pickup"
			PickUpGui.Adornee = item
			ObjectName.Text = item.Name
			PickUpGui.Enabled = true
		elseif mouse.Target:FindFirstChild("LootBox") then
			local item = mouse.Target
			EtoPickup.Text = "E - Pickup"
			PickUpGui.Adornee = item
			ObjectName.Text = item.Name
			PickUpGui.Enabled = true
		elseif mouse.Target:FindFirstChild("BankMan") then
			local item = mouse.Target
			EtoPickup.Text = "E - Interact"
			PickUpGui.Adornee = item
			ObjectName.Text = "Bank"
			PickUpGui.Enabled = true
		elseif mouse.Target:FindFirstChild("StoreMan") then
			local item = mouse.Target
			EtoPickup.Text = "E - Interact"
			PickUpGui.Adornee = item
			ObjectName.Text = "Bank"
			PickUpGui.Enabled = true
		elseif mouse.Target:FindFirstChild("BlackSmith") then
			local item = mouse.Target
			EtoPickup.Text = "E - Interact"
			PickUpGui.Adornee = item
			ObjectName.Text = "BlackSmith"
			PickUpGui.Enabled = true
			
		elseif mouse.Target:FindFirstChild("Buyable") then
			local item = mouse.Target
			EtoPickup.Text = "E - Buy"
			PickUpGui.Adornee = item
			ObjectName.Text = item.Name
			PickUpGui.Enabled = true
		else
			EtoPickup.Text = "E - Pickup"
			PickUpGui.Adornee = nil
			PickUpGui.Enabled = false
		end
	end
end)
1 Like
#2

I’m not sure, it seems fine to me. I’ll check back over it later and see if there are any ways to improve on it and make it run smoother. :grin:

#3

As said above, your script seems totally fine the way it is and really can’t be simplified, however you could make a function for everything that happens inside the if statements.

#4

One thing about improving code is figuring out what you can factor out. A lot of your branches have the same code, so you can take advantage of that and just shove the differences into the table for easy lookup. Additionally, in places where you don’t need some extra logic running past a big chunk of code you can use a guard clause to avoid the extra scope.

Here’s what my example refactor might look like:

local Interactive = {
    Pickable = 'E - Pickup',
    LootBox = 'E - Pickup',
    BankMan = 'E - Interact',
    StoreMan = 'E - Interact',
    BlackSmith = 'E - Interact',
    Buyable = 'E - Buy',
}

-- UIS.InputChanged seems like a weird place to check for mouse.Target
-- This fires for every key up and down
UIS.InputChanged:Connect(function(input)
    local item = mouse.Target

    if not item then -- guard clause to avoid whole new scope
        return
    end

    local ok_text

    for name, text in pairs(Interactive) do
        if item:FindFirstChild(name) then
            ok_text = text -- finds whichever item from the list is present
            break
        end
    end

    -- if an item is present, just do the single branch for it, otherwise disable the pickup gui
    if ok_text then
        EtoPickup.Text = ok_text
        ObjectName.Text = item.Name
        PickUpGui.Adornee = item
        PickUpGui.Enabled = true
    else
        PickUpGui.Adornee = nil
        PickUpGui.Enabled = false
    end
end)

I also would suggest approaching it differently however. Right now you check for each item you could be interacting with, why not use something like a tag? If mouse.Target contains an Item tag, you know it’ll always be something you pick up instead of interact with. In that tag you could also maybe store information about the item. For an NPC, the NPC tag could be present and the value could be the internal name/ID of the NPC for example. This would avoid repeating the same code for BankMan, StoreMan, and such.

7 Likes