Feedback on bulk item selling system that causes lag spikes

[minionkim]

Include a standalone, bare-bones rbxl file with only the code you want reviewed.

• Code Review is for reviewing specific parts of your code, and not your whole game.
• Code Review is intended for improving already-working code. If you need help debugging your code, please use Help and Feedback > Scripting Support

Provide an overview of:

• What does the code do and what are you not satisfied with?
• What potential improvements have you considered?
• How (specifically) do you want to improve the code?

function SeedShopModule.SellItems(player, SeedTable)
–[[Seed Table is a table with index of item name annd amount being requesuted to be sold
For Example:
{
[“Carrot”] = 10,
[“Bean”] = 5,
}

]]		
local TotalCoinsToSell = 0 -- variable for amount of coins given to player after selling all items
for Item, Amount in SeedTable do
	local Counter = 0
	local ItemFind = SS.AllItems:FindFirstChild(Item) -- accesses item in all items folder where i can see item details such as attributes
	if not ItemFind then
		warn("itemnotfound")
	return end
	local SellPrice = ItemFind:GetAttribute("SellPrice")
	--Gets the sellprice attribute which is the amount of money the item sells for individually. number value
	
	for i, tool in player.Backpack:GetChildren() do -- loops for all items inside player backpack. breaks if counter is reaches amount requested to be sold
		if Counter >= Amount then
			break
		end
		if tool.Name == Item  then
			Counter += 1
			tool:Destroy()
			TotalCoinsToSell += SellPrice
		end
	end
	if Counter < Amount then
		local PlayerCheck = player.Character:FindFirstChildWhichIsA("Tool") -- if requesuted amount not reached, checks player character for item to see if they are holding
		if PlayerCheck and PlayerCheck.Name == Item then
			Counter += 1
			PlayerCheck:Destroy()
			TotalCoinsToSell += SellPrice		
			if Counter < Amount then
				warn("Player request sold more than they had")
			end
		end
	end

end

-- finally adds gold to player
DataManager.AddGold(player, TotalCoinsToSell)

end

Is there a way to optimize this selling system? I get lag spikes if I bulk sell too many items. The main problem is the amount of loops if playerbackpack already has a lot of items

[TheGrimDeathZombie (Reapimus)]

@minionkim

Is there a way to optimize this selling system?

Instead of using GetChildren or :FindFirstChildWhichIsA it might be more beneficial for you to keep your own cache of every item the player has in both their inventory and on their character using events, which could end up looking something like this:

local Players = game:GetService("Players")

local PlayerItems = {}

function playerAdded(player: Player)
    PlayerItems[player] = {}

    for _, v in player.Backpack:GetChildren() do
        PlayerItems[player][v] = v.Name
    end
    
    player.Backpack.ChildAdded:Connect(function(child)
        PlayerItems[player][child] = child.Name
    end)
    
    player.Backpack.ChildRemoving:Connect(function(child)
        if child.Parent == player.Character then return end
    
        PlayerItems[player][child] = nil
    end)
end

Then you could simply do a for tool, name in PlayerItems[player] do loop to check all of the items without having to constantly query the engine since that is likely where your issue lies if it happens when there’s a lot of items in their inventory.

[neOfficialProTv (Andrei)]

Basically the issue is you’re doing way too many loops man.Like, if someone has a ton of stuff in their backpack and wants to sell multiple item types, you’re checking the same items over and over again which is super inefficient

Here’s what you should do instead, just go through the backpack once and grab everything you need:

function SeedShopModule.SellItems(player, SeedTable)
    local TotalCoinsToSell = 0
    local stillNeed = {} -- what we still gotta sell
    
    -- copy the original request
    for item, amount in SeedTable do
        stillNeed[item] = amount
    end
    
    -- one loop through everything in backpack
    for _, tool in player.Backpack:GetChildren() do
        local itemName = tool.Name
        if stillNeed[itemName] and stillNeed[itemName] > 0 then
            local itemData = SS.AllItems:FindFirstChild(itemName)
            if itemData then
                tool:Destroy()
                TotalCoinsToSell += itemData:GetAttribute("SellPrice")
                stillNeed[itemName] -= 1
            end
        end
    end
    
    -- check if theyre holding something too
    local heldTool = player.Character:FindFirstChildWhichIsA("Tool")
    if heldTool and stillNeed[heldTool.Name] and stillNeed[heldTool.Name] > 0 then
        local itemData = SS.AllItems:FindFirstChild(heldTool.Name)
        if itemData then
            heldTool:Destroy()
            TotalCoinsToSell += itemData:GetAttribute("SellPrice")
        end
    end
    
    DataManager.AddGold(player, TotalCoinsToSell)
end