Help Improving Shop Code

[simplyjustbased]

Alright, finally set up some sort of basic shop system from scratch and now wondering how i can clean up / patch up some of the code

It all works, with some of it erroring sometimes

Loading the character in with equipped guns

Basically how im doing it currently is having a folder with alot of stats and stuff to hold nearly all the information

Now, currently i have a not so great spawning in system that only got worse with equipping guns

local SpawnEvent = game.ReplicatedStorage.Events.SpawnEvent
local GunsHolder = game.ReplicatedStorage.ItemHolder.Guns
local ArmorHolder = game.ReplicatedStorage.ItemHolder.Armors
local EnemyHolder = game.ReplicatedStorage.ItemHolder.Enemys

local function SpawnCharacter(Player)
	if game.Workspace:FindFirstChild(Player.Name) then return end
	local plrDataFolder = game.ReplicatedStorage.InvHolder:FindFirstChild(Player.Name .. "DataFolder")
	local Primary = plrDataFolder.Game.EquippedPrimary.Value
	local Secondary = plrDataFolder.Game.EquippedSecondary.Value
	Player:LoadCharacter()
	
	if Primary ~= nil then
		local Pgun = GunsHolder:FindFirstChild(Primary)
		if Pgun then
			local D = Pgun:Clone()
			D.Parent = Player.Backpack
		end
	end
	
	if Secondary ~= nil then
		local Sgun = GunsHolder:FindFirstChild(Secondary)
		if Sgun then
			local D = Sgun:Clone()
			D.Parent = Player.Backpack
		end
	end
	
end

SpawnEvent.OnServerEvent:Connect(SpawnCharacter)

Aka if i were to continue this i would have alot of if and statements

Just to include it heres the code for creating players folders

local BaseClone = game.ReplicatedStorage["#DataFolder"]
local InventoryHolder = game.ReplicatedStorage.InvHolder
local Inventorys = {}

local function CreationFolder(Player)
	if InventoryHolder:FindFirstChild(Player.Name .. "DataFolder") then return end
	local PlayersData = BaseClone:Clone()
	PlayersData.Name = Player.Name .. "DataFolder"
	PlayersData.Parent = InventoryHolder
	Inventorys[Player.Name] = PlayersData
end

local function DeletionFolder(Player)
	if not InventoryHolder:FindFirstChild(Player.Name .. "DataFolder") then return end
	local Inv = Inventorys[Player.Name]
	Inv:Destroy()
	Inventorys[Player.Name] = nil
end

game.Players.PlayerAdded:Connect(CreationFolder)
game.Players.PlayerRemoving:Connect(DeletionFolder)

Current selected gun in shop

Ok for here, its all jumbled up

Basically what this is spose to do

For the buttons its simple

local GunGui = script.Parent
local GunModule = require(game.ReplicatedStorage.Modules.GunShop)
local ShopEvent = game.ReplicatedStorage.Events:WaitForChild("ShopEvent")

repeat wait() until #GunGui:GetChildren() > 0 -- temporarily here until i get a IsDataLoaded Value set up

local function GuiClicked(Gui, Gun)
	if Gui and GunModule[Gun] then
		ShopEvent:Fire(Gun)
	end
end


for i, v in ipairs(GunGui:GetChildren()) do
	if v.ClassName == "TextButton" or v.ClassName == "ImageButton" then
		v.Activated:Connect(function()
			GuiClicked(v, v.Gun.Value)
		end)
	end
end

Not the best but gets it done

But the selected code on the other hand

local SelGui = script.Parent
local player = game.Players.LocalPlayer
local PlayerFolder = game.ReplicatedStorage.InvHolder:FindFirstChild(player.Name .. "DataFolder")
local GunMod = require(game.ReplicatedStorage.Modules.GunShop)
local GunEvent = game.ReplicatedStorage.Events:WaitForChild("ShopEvent")
local PurchaseEvent = game.ReplicatedStorage.Events:WaitForChild("PurchaseEvent")
local EquipEvent = game.ReplicatedStorage.Events:WaitForChild("EquipEvent")
local Image = SelGui["GunPicture(TEMP)"]
local CostText = SelGui.CostLabel
local ReqText = SelGui.ReqLevelLabel
local PurchaseButton = SelGui.PurchaseButton
local EquipButton = SelGui.EquipButton
local GunSelected = nil

local function BuyGun(Gun)
	if not Gun then return "No Gun Selected" end
	if not GunMod[Gun] then return "NoGun" end
	PurchaseEvent:FireServer(Gun)
end

local function EquipGun(Gun)
	if not Gun then return "Still No Gun" end
	if not GunMod[Gun] then return "Gun Doesnt Exist" end
	EquipEvent:FireServer(Gun)
end

GunEvent.Event:Connect(function(Gun)
	if Gun == GunSelected then
		GunSelected = nil
	else
		GunSelected = Gun
	end
end)

PurchaseButton.Activated:Connect(function()
	BuyGun(GunSelected)
end)

EquipButton.Activated:Connect(function()
	EquipGun(GunSelected)
end)


while wait() do
	if GunSelected ~= nil then
		Image.Image = if GunMod[GunSelected].PictureId then "http://www.roblox.com/asset/?id=" .. GunMod[GunSelected].PictureId else 0
		CostText.Text = GunMod[GunSelected].Cost .. "$"
		ReqText.Text = GunMod[GunSelected].LevelReq
		if PlayerFolder.ShopOwned[GunMod[GunSelected].GunType .. "Guns"]:FindFirstChild(GunSelected) then
			local GunType = GunMod[GunSelected].GunType
			if PlayerFolder.Game["Equipped" .. GunType].Value == GunSelected then
				EquipButton.Visible = true
				EquipButton.Text = "UnEquip"
			else
				EquipButton.Visible = true
				EquipButton.Text = "Equip"
			end
		else
			PurchaseButton.Visible = true	
		end
	else
		Image.Image = 0
		CostText.Text = " "
		ReqText.Text = " "
		PurchaseButton.Visible = false
		EquipButton.Visible = false 
	end
end

Its not too good,
Also may i add that this

if PlayerFolder.ShopOwned[GunMod[GunSelected].GunType .. "Guns"]:FindFirstChild(GunSelected) then

Just sometimes works, but also sometimes just errors randomly

For here, mainly wondering how i can get this cleaned up, and stop that from randomly erroring

Shop / Equip Handler

This, well it isnt really good
It works without error, but looking on mainly how i can get it cleaned up, because its all over the place (In my opinion)

local PurchaseEvent = game.ReplicatedStorage.Events:WaitForChild("PurchaseEvent")
local EquipEvent = game.ReplicatedStorage.Events:WaitForChild("EquipEvent")
local GunMod = require(game.ReplicatedStorage.Modules.GunShop)

local function CheckIfCanPurchase(Player, Gun, PlrFolder)
	if not GunMod[Gun] then return false end
	if not PlrFolder then return false end
	if PlrFolder.CashValue.Value < GunMod[Gun].Cost then return false end
	if PlrFolder.ShopOwned[GunMod[Gun].GunType .. "Guns"]:FindFirstChild(Gun) then return false end
	--if PlrFolder.LevelValue.Value < GunMod[Gun].LevelReq then return false end
	return true
end

local function CheckifGunExists(plrF, Gun)
	if not plrF.ShopOwned[GunMod[Gun].GunType .. "Guns"]:FindFirstChild(Gun) then return false end
	return true
end

local function PurchaseGun(Player, Gun)
	local PlrFolder = game.ReplicatedStorage.InvHolder:FindFirstChild(Player.Name .. "DataFolder")
	local Check = CheckIfCanPurchase(Player, Gun, PlrFolder)
	
	if Check == true then
		PlrFolder.CashValue.Value -= GunMod[Gun].Cost
		local GunVal = Instance.new("StringValue", PlrFolder.ShopOwned[GunMod[Gun].GunType .. "Guns"])
		GunVal.Name = Gun
		print("PlayerBoughtGun")
	else
		print("PlayerDidntBuy")
	end
end

local function EquipGun(Player, Gun)
	local PlayerFolder = game.ReplicatedStorage.InvHolder:FindFirstChild(Player.Name .. "DataFolder")
	local Check = CheckifGunExists(PlayerFolder, Gun)
	if Check == false then return false end
	
	if PlayerFolder.Game.EquippedPrimary.Value == Gun then
		PlayerFolder.Game.EquippedPrimary.Value = " "
	else
		PlayerFolder.Game.EquippedPrimary.Value = Gun
	end
end

PurchaseEvent.OnServerEvent:Connect(PurchaseGun)
EquipEvent.OnServerEvent:Connect(EquipGun)

Quick view on what it currently is like

[uwuCulturist (fox)]

@simplyjustbased

Player:LoadCharacter()

Just skimming through this right now, do not let players run this function through a remote with no cooldown.
To crash servers in your game, all one has to do is delete their character and then spam fire the remote.
LoadCharacter takes a lot of resources so you should never let a player spam it.

[FarFromLittle]

@simplyjustbased

local PlayerFolder = game.ReplicatedStorage.InvHolder:FindFirstChild(player.Name .. "DataFolder")
local GunMod = require(game.ReplicatedStorage.Modules.GunShop)
local GunEvent = game.ReplicatedStorage.Events:WaitForChild("ShopEvent")
local PurchaseEvent = game.ReplicatedStorage.Events:WaitForChild("PurchaseEvent")

This local script should wait for InvHolder, Modules, and Events before attempting to access it’s children, also WaitForChild should be used instead of FindFirstChild for the player’s folder.

[simplyjustbased]

@uwuCulturist

To crash servers in your game, all one has to do is delete their character and then spam fire the remote.

Even with the stuff i tried to implement to stop spamming,its still gonna happen?

@FarFromLittle

This local script should wait for InvHolder , Modules , and Events before attempting to access it’s children, also WaitForChild should be used instead of FindFirstChild for the player’s folder.

Alright ill change up my code and let you know how it goes

[uwuCulturist (fox)]

@simplyjustbased

Even with the stuff i tried to implement to stop spamming,its still gonna happen?

If you are trying to implement a ratelimit, here you go

local ratelimits = {}

local SpawnEvent = game.ReplicatedStorage.Events.SpawnEvent
local GunsHolder = game.ReplicatedStorage.ItemHolder.Guns
local ArmorHolder = game.ReplicatedStorage.ItemHolder.Armors
local EnemyHolder = game.ReplicatedStorage.ItemHolder.Enemys

local function SpawnCharacter(Player)
    if not ratelimits[Player] then
        ratelimits[Player] = 0
    end
    if tick() - ratelimits[Player] < 2 then -- if it has been less than 2 seconds since the last spawn p much
        return
    end
    ratelimits[Player] = tick()
	if game.Workspace:FindFirstChild(Player.Name) then return end
	local plrDataFolder = game.ReplicatedStorage.InvHolder:FindFirstChild(Player.Name .. "DataFolder")
	local Primary = plrDataFolder.Game.EquippedPrimary.Value
	local Secondary = plrDataFolder.Game.EquippedSecondary.Value
	Player:LoadCharacter()
	
	if Primary then
		local Pgun = GunsHolder:FindFirstChild(Primary)
		if Pgun then
			local D = Pgun:Clone()
			D.Parent = Player.Backpack
		end
	end
	
	if Secondary then
		local Sgun = GunsHolder:FindFirstChild(Secondary)
		if Sgun then
			local D = Sgun:Clone()
			D.Parent = Player.Backpack
		end
	end
	
end

SpawnEvent.OnServerEvent:Connect(SpawnCharacter)

Also the “~= nil” thing was unnescessary so I cut it out

[simplyjustbased]

@uwuCulturist

If you are trying to implement a ratelimit, here you go

local ratelimits = {}

local SpawnEvent = game.ReplicatedStorage.Events.SpawnEvent
local GunsHolder = game.ReplicatedStorage.ItemHolder.Guns
local ArmorHolder = game.ReplicatedStorage.ItemHolder.Armors
local EnemyHolder = game.ReplicatedStorage.ItemHolder.Enemys

local function SpawnCharacter(Player)
    if not ratelimits[Player] then
        ratelimits[Player] = 0
    end
    if tick() - ratelimits[Player] < 2 then -- if it has been less than 2 seconds since the last spawn p much
        return
    end
    ratelimits[Player] = tick()
	if game.Workspace:FindFirstChild(Player.Name) then return end
	local plrDataFolder = game.ReplicatedStorage.InvHolder:FindFirstChild(Player.Name .. "DataFolder")
	local Primary = plrDataFolder.Game.EquippedPrimary.Value
	local Secondary = plrDataFolder.Game.EquippedSecondary.Value
	Player:LoadCharacter()
	
	if Primary then
		local Pgun = GunsHolder:FindFirstChild(Primary)
		if Pgun then
			local D = Pgun:Clone()
			D.Parent = Player.Backpack
		end
	end
	
	if Secondary then
		local Sgun = GunsHolder:FindFirstChild(Secondary)
		if Sgun then
			local D = Sgun:Clone()
			D.Parent = Player.Backpack
		end
	end
	
end

SpawnEvent.OnServerEvent:Connect(SpawnCharacter)

Damn, i thought the

if game.Workspace:FindFirstChild(Player.Name) then return end

Would be enough for it to not break that hard.

@FarFromLittle

This local script should wait for InvHolder , Modules , and Events before attempting to access it’s children, also WaitForChild should be used instead of FindFirstChild for the player’s folder.

Not sure if this was exactly what you were thinking, but i changed it to this

local GunMod = require(game.ReplicatedStorage.Modules.GunShop)
local player = game.Players.LocalPlayer
local Events = game.ReplicatedStorage:WaitForChild("Events")
local Inventorys = game.ReplicatedStorage:WaitForChild("InvHolder")
local GunEvent = Events:WaitForChild("ShopEvent")
local PurchaseEvent = Events:WaitForChild("PurchaseEvent")
local EquipEvent = Events:WaitForChild("EquipEvent")
local PlayerFolder = Inventorys:WaitForChild(player.Name .. "DataFolder")

[FarFromLittle]

@simplyjustbased

Not sure if this was exactly what you were thinking, but i changed it to this

That’s it, but there’s more I didn’t mention. Since you gave it a shot, I’ll show you what I would do.

local SelGui = script.Parent
local player = game.Players.LocalPlayer

local Inventories = game.ReplicatedStorage:WaitForChild("InvHolder")
local Modules = game.ReplicatedStorage:WaitForChild("Modules")
local Events = game.ReplicatedStorage:WaitForChild("Events")

local GunMod = require(Modules:WaitForChild("GunShop"))
local PlayerFolder = Inventories:FindFirstChild(player.Name .. "DataFolder")

local GunEvent = Events:WaitForChild("ShopEvent")
local PurchaseEvent = Events:WaitForChild("PurchaseEvent")
local EquipEvent = Events:WaitForChild("EquipEvent")

local Image = SelGui:WaitForChild("GunPicture(TEMP)")
local CostText = SelGui:WaitForChild("CostLabel")
local ReqText = SelGui:WaitForChild("ReqLevelLabel")
local PurchaseButton = SelGui:WaitForChild("PurchaseButton")
local EquipButton = SelGui:WaitForChild("EquipButton")

local GunSelected = nil

Hope it helps.