Revieuw my backpackscript please

Help and Feedback Code Review
#1

this script is a Backpack Stack system.
before I put it in any of my games i thought that maybe the keen eye of the Roblox community can see if there are any bugs I might have missed

when you pick up an item it checks for attributes like “Name” and “Quantity”
if they are not present the tool will get those.

when you store the tool in your backpack it will search for other tools with the same Name “Attribute” it will increase the quantity on one tool originally in the backpack and remove the new tool.

to indicate the quantity the name of the tool will change to <name - > quantity
(if the tool has an image this wont show)

when selecting a tool from your backpack it checks the quantity attribute. if it is more than 1 it will push the remaining back into the inventory (so you only have 1 tool active at the time)

The Code 
--[[
 ____  ___  ____ 
(  _ \/ __)(  _ \
 ) _ <\__ \ )___/
(____/(___/(__)  
BackpackStackProject
V0.0.3
By: Boeljoet 
]]

local Players = game:GetService("Players")

local function rename(tool)
	if tool:GetAttribute("Quantity") > 1 then
		tool.Name = tool:GetAttribute("Name").." - "..tool:GetAttribute("Quantity")
	else
		tool.Name = tool:GetAttribute("Name")
	end
end

local function Checktool(player,tool)
	local info ={["Name"] = tool.Name,["Quantity"] = 1}
	for i,v in pairs(info) do
		if not tool:GetAttribute(i) then
			tool:SetAttribute(i,v)
		end
	end
end

local function search(part,folder)
	local Name = part:GetAttribute("Name")
	for i,v in pairs(folder:GetChildren()) do
		if v:GetAttribute("Name") == Name then
			return v
		end
	end
	return nil
end

local function changerespawn (player,char)
	local StarterGear = player.StarterGear
	local Backpack = player:WaitForChild("Backpack")
	for i,v in pairs(Backpack:GetChildren()) do
		local startertool = search(v,StarterGear)
		if startertool then
			startertool:SetAttribute("Quantity",v:GetAttribute("Quantity"))
		else
			v:Clone().Parent = player.StarterGear
		end
	end
	for i,v in pairs(StarterGear:GetChildren()) do
		local startertool = search(v,Backpack)
		if startertool then else
			v:Destroy()
		end
	end
	local chartool = char:FindFirstChildWhichIsA("Tool")
	if chartool then
		local resptool = search(chartool,StarterGear)
		if resptool then
			resptool:SetAttribute("Quantity",resptool:GetAttribute("Quantity")+1)
		else
			chartool:Clone().Parent = player.StarterGear
		end
	end
	for i,v in pairs(StarterGear:GetChildren()) do
		rename(v)
	end
end

local function itemRemoved (player,tool)
	if tool:GetAttribute("Quantity") > 1 and tool.Parent == player.Character then
		local newtool = tool:Clone()
		newtool:SetAttribute("Quantity",tool:GetAttribute("Quantity")-1)
		wait()
		newtool.Parent = player.Backpack
		tool:SetAttribute("Quantity",1)
		rename(tool)
	end	
end

local function itemAdded(player,tool)
	local stack = nil
	for i,v in pairs(player.Backpack:GetChildren()) do
		if v ~= tool and v:GetAttribute("Name") == tool:GetAttribute("Name") then
			stack = v
		end
	end
	if stack then
		stack:SetAttribute("Quantity",stack:GetAttribute("Quantity")+ tool:GetAttribute("Quantity"))
		wait()
		tool:Destroy()
		rename(stack)
	else
		rename(tool)
	end
end

function activate(player)
	player.CharacterAdded:Connect(function(character)
		local backpack = player.Backpack
		local C1 = backpack.ChildAdded:Connect(function(tool)
			itemAdded(player,tool)
			changerespawn (player,character)
		end)

		local C2 = backpack.ChildRemoved:Connect(function(tool)
			itemRemoved (player,tool)
			changerespawn (player,character)
		end)
		local C3 =character.ChildAdded:Connect(function(tool)
			if tool:IsA("Tool") then
				Checktool(player,tool)
				changerespawn (player,character)
			end
		end)
		local C4 =character.ChildRemoved:Connect(function(tool)
			if tool:IsA("Tool") then
				changerespawn (player,character)
			end
		end)
		character:WaitForChild("Humanoid").Died:Connect(function()
			C1:disconnect()
			C2:disconnect()
			C3:disconnect()
			C4:disconnect()
		end)
	end)
end

Players.PlayerAdded:Connect(activate)
for i,v in pairs(Players:GetPlayers()) do
	activate(v)
end

i am asking for a gereal revieuw to see for any bugs, optimisations or anything you can find that will help improve the project

1 Like
#2

In the itemadded function:

Using wait() can slow down your scripts and you should avoid using it. Use game:GetService("RunService").Heartbeat:Wait() instead (it is two times faster than wait()). Read this community tutorial to learn more about why you should avoid using wait():

Overall, I like how you disconnect your events, which is a really good habit. Keep up the great work!

2 Likes
#3

rename could be renamed (no pun intended) to updateName. rename feels equivalent to setName, while updateName better describes what the function does, IMO.

Checktool has upper case first letter? Pick a scheme and stick with it. It should also probably be changed to setupToolAttributes to better describe what it does.

search(v,Backpack)

Space after commas: search(v, Backpack)

Space aorund binary operators: tool:GetAttribute("Name").." - " becomes tool:GetAttribute("Name") .. " - "

search is a very generic description of what that function does, not technically wrong but not specific enough to actually be useful either. I’d rename it to findFirstChildWithSameNameAttr or maybe findChildWithSameNameAttr.

if startertool then else
    v:Destroy()
end

if not startertool then v:Destroy() end is more readable. Generally it’s best to avoid “clever tricks”. I also think it would be better to throw an error here,

startertool:SetAttribute("Quantity",v:GetAttribute("Quantity")) this line does nothing. Did you mean startertool:SetAttribute("Quantity",v:GetAttribute("Quantity") + 1)?

for i,v in pairs(Backpack:GetChildren()) do if you don’t use a for loop variable, replace it with _. E.g. for _, v in pairs(Backpack:GetChildren()) do.

It’s not so bad here, but avoid using single letter variable names like v, unless your use case is generic enough that you can’t know what kind of thing v is so v just refers to “a thing in a list”. E.g. rename v in for _, v in pairs(Backpack:GetChildren()) do to backpackTool or just tool.

I can’t guess what changeRespawn is supposed to do. Rename it to describe what it does, add comments to explain what it does, and probably split it into smaller functions.

2 Likes