Any way to ake the Items Manager more reliable (Client)

Hello! I created an items manager. This is the client side script. Is it possible to make this code more reliable?

The code

local uis = game:GetService("UserInputService")

local itemsR = {
-- Rarity of the items go here. 
}

local itemIDs = {
-- Items image ID's go here
}

local names = {
-- Items names go here
}

local rarity = {}

for i, v in next, itemsR do
	for e = 1, v do
		table.insert(rarity, tostring(i))
	end
end

function getItem()
	local item = rarity[math.random(1,#rarity)]
	return item
end

--Item Adding
game.ReplicatedStorage.PlayerRemotesFolder:WaitForChild(game.Players.LocalPlayer.Name.."AddItem").OnClientEvent:Connect(function(items)
	local selected = getItem()
	local amount = 40
	if items.item1.Value == "None" and not items.Choosing.Value then
		--Player does not have item 1
		local x = 1
		if script.Parent.ItemBoxLoop.Playing == false then
			script.Parent.Items.Visible = true
			script.Parent.ItemBoxLoop:Play()
		end
		for i = 1, amount do
			if x == #itemIDs then
				x = 1
			end
			script.Parent.Items.Item1.Item.Image = itemIDs[x]
			x += 1
			wait(0.1)
		end
		script.Parent.ItemBoxLoop:Stop()
		script.Parent.NewItem:Play()
		script.Parent.Items.Item1.Item.Image = selected
		game.ReplicatedStorage.ClientSendBackItem:FireServer(selected, 1, names[selected])
	elseif items.item2.Value == "None" and not items.Choosing2.Value then
		--Player does have item 1, meaning that they need Item 2
		local x = 1
		if script.Parent.ItemBoxLoop.Playing == false then
			script.Parent.ItemBoxLoop:Play()
		end
		for i = 1, amount do
			if x == #itemIDs then
				x = 1
			end
			script.Parent.Items.Item2.Item.Image = itemIDs[x]
			x += 1
			wait(0.1)
		end
		script.Parent.ItemBoxLoop:Stop()
		script.Parent.NewItem:Play()
		script.Parent.Items.Item2.Item.Image = selected
		game.ReplicatedStorage.ClientSendBackItem:FireServer(selected, 2, names[selected])
	end
end)

--Player Uses Item
uis.InputBegan:Connect(function(input, gamePr)
	if input.KeyCode == Enum.KeyCode.R then
		if not gamePr then
			--Use Item
			game.ReplicatedStorage.ItemUseEvent:FireServer()
		end
	end
end)

script.Parent.Items.Hotkey.TextButton.MouseButton1Click:Connect(function()
	game.ReplicatedStorage.ItemUseEvent:FireServer()
end)

game.ReplicatedStorage.PlayerRemotesFolder:FindFirstChild(game.Players.LocalPlayer.Name.."SwitchItem").OnClientEvent:Connect(function()
	local ItemsFolder = game.ReplicatedStorage.PlayerItems:FindFirstChild(game.Players.LocalPlayer.Name.."Items")
	if ItemsFolder then
		if ItemsFolder.item1.Value == "None" then
			script.Parent.Items.Visible = false
			script.Parent.Items.Item1.Item.Image = "rbxassetid://6362164005"
		else
			for i, val in pairs(names) do
				if val == ItemsFolder.item1.Value then
					script.Parent.Items.Item1.Item.Image = i
				end
			end
		end
		if ItemsFolder.item2.Value == "None" then
			script.Parent.Items.Item2.Item.Image = "rbxassetid://6362164005"
		end
	end
end)

If you find a flaw with this code, please feel free to let me know! Thanks, WE

Server Side code

1 Like

Just some feedback, would highly suggest putting those two topics together!

And you can use details to hide each script
Like this

Server Script
Client Script

Anyways lets get right into it!

The first thing that stood out to me is this

I generally don’t like to do this as you are re-defining the item in each table. You generally are better off trying to do this

local data = {
   --itemName = {rarity = 'rare', imageId = 'imageId' 
}

So when you create loops, it is kind of confusing to the outside. You can create loops with better variables and use _ to represent variables that you aren’t using.

Fun fact: i,v stands for index,value

for _, value in ipairs(otherTable) do 
    for index, somethingElse in ipairs(value) do
        --
    end
end

Next

As good practice use game:GetService(“ReplicatedStorage”) instead :stuck_out_tongue:


Unnecessary x variable, since you can just use the index ( i )


Personally wouldn’t use UserInputService b/c you can trigger things with your chat open. I would suggest ContextActionService instead


Recommend using variable caching

local root = script.Parent:WaitForChild("Items")
local Hotkey = root:WaitForChild("Hotkey")
-- etc

Also store the locaiton of remote events in variables too!


A personal player remote event is kind of excessive, since you can use remote events to fire a single client.

remote:FireClient(PlayerInstance, data)

That’s some of the feedback I can give! Although there is a lot of it, so you might be inclined to redo the script :sweat_smile:. But all this is up to you!

1 Like