Bowling Shop System Feedback

Help and Feedback Code Review
#1

Hello! I recently finished my bowling shop system but some part of me is telling me that it isn’t secure, or very reliable. I was wondering if any of you had any feedback on how I could make it better! Anything helps! I have a few buttons that are locked for 3 seconds at a time AFTER the user presses the confirm button so if they go and click cancel after they purchase and go to buy another item within that 3-second point, it won’t disappear or do something unwanted.

local ItemModule = require(game.ReplicatedStorage.GameRunner.Modules.ItemInfo)
local CurrentSelectedItem = nil
local ItemClass = nil
local Locked = false
local CancelLocked = false
local BuyLocked = false

function Buy()
	if CurrentSelectedItem ~= nil then
		Locked = true
		script.Parent.Shop.ConfirmPurchase.Visible = true
		script.Parent.Shop.ConfirmPurchase.TextLabel.Text = "Confirm Purchase of "..CurrentSelectedItem["Name"].." for ".. CurrentSelectedItem["Price"].. "?"
	end
end
script.Parent.Shop.Buy.MouseButton1Click:Connect(Buy)

function CancelPurchase()
	if CancelLocked == false then
	script.Parent.Shop.ConfirmPurchase.Visible = false
	Locked = false
	end
end
script.Parent.Shop.ConfirmPurchase.Cancel.MouseButton1Click:Connect(CancelPurchase)

function ProceedPurchase()
	local FoundItem = false
	for i,v in pairs(game.Players.LocalPlayer.Inventory:GetDescendants()) do
		if v.Name == CurrentSelectedItem["Name"] then
			FoundItem = true
		end
	end
	CancelLocked = true
	BuyLocked = true
		if FoundItem == false and game.Players.LocalPlayer.Money.Value >= CurrentSelectedItem["Price"] then
			game.ReplicatedStorage.GameRunner.Events.Purchase:FireServer(ItemClass, CurrentSelectedItem)
			script.Parent.Shop.ConfirmPurchase.TextLabel.Text = "Successfully purchased item "..CurrentSelectedItem["Name"].." for "..CurrentSelectedItem["Price"].."!"
			wait(3)
			script.Parent.Shop.ConfirmPurchase.Visible = false
			BuyLocked = false
			CancelLocked = false
			Locked = false
		elseif FoundItem == true then
			script.Parent.Shop.ConfirmPurchase.TextLabel.Text = "You already own "..CurrentSelectedItem["Name"].."!"
			wait(3)
			script.Parent.Shop.ConfirmPurchase.Visible = false
			BuyLocked = false
			CancelLocked = false
		Locked = false
		else
		script.Parent.Shop.ConfirmPurchase.TextLabel.Text = "You don't have enough money for "..CurrentSelectedItem["Name"].."!"
		wait(3)
		script.Parent.Shop.ConfirmPurchase.Visible = false
		BuyLocked = false
		CancelLocked = false
		Locked = false
		end
	end
script.Parent.Shop.ConfirmPurchase.Confirm.MouseButton1Click:Connect(ProceedPurchase)

for i,v in pairs(ItemModule.BowlingBalls) do
	local F = script.Parent.Shop.BowlingBalls.Default:Clone()
	local C = v["ModelLink"]:Clone()
	for o,j in pairs(C:GetChildren()) do
		if v.ClassName == not "SpecialMesh" then
			v:Destroy()
		end
	end
	C.Anchored = true
	C.Position = Vector3.new(0, 0, -1.5)
	C.Parent = F.ItemImage
	F.Parent = script.Parent.Shop.BowlingBalls
	local ConvertPrice = tostring(v["Price"])
	F.Name = ConvertPrice
	function Clicked()
		if Locked == false then
		local c = C:Clone()
		for a,h in pairs (script.Parent.Shop.SelectedItem.ItemImageBackground.ItemImage:GetChildren()) do
			h:Destroy()
		end
		CurrentSelectedItem = v
		ItemClass = "BowlingBalls"
		script.Parent.Shop.Buy.Visible = true
		c.Parent = script.Parent.Shop.SelectedItem.ItemImageBackground.ItemImage
		script.Parent.Shop.SelectedItem.ItemImageBackground.ItemName.Text = v["Name"]
		script.Parent.Shop.SelectedItem.ItemInfo.ItemDescription.Text = v["ItemDescription"].." ".."This ball has "..v["Hook"].. " Hook Points and "..v["Speed"].." Speed Points."
		end
	end
	F.Button.MouseButton1Click:Connect(Clicked)
end
script.Parent.Shop.BowlingBalls.Default:Destroy()
#2 
for i,v in pairs(game.Players.LocalPlayer.Inventory:GetDescendants()) do
		if v.Name == CurrentSelectedItem["Name"] then
			FoundItem = true
		end
	end

that part right there is just screaming for people to exploit

1 Like
#3

You would have to use remote functions

#4

This part is to check if the player already owns that item.

#5

Yes but hackers could clone something out of the replicated storage then put it into the players inventory

1 Like
#6

Ah, so maybe I instead put the player’s inventory folder in the ServerScriptService/Server Storage?

#7

No, you could make a remote event and then put it into replicated storage, so you would fire it from the local side and then on the server side, you would check the players inventory from the server to see if the item is actually there. If its not there then you kick the player for hacking

1 Like
#8

but if the item is actually there, then you would make Found Item to true

#9

One small thing I noticed besides what @VectorCode3 already pointed out is that you should switch your events from .MouseButton1Click to .InputBegan. This would require a bit of code changes, for example:

script.Parent.Shop.ConfirmPurchase.Confirm.InputBegan:Connect(function(inputObject)
    if inputObject.UserInputType == Enum.UserInputType.MouseButton1Down or inputObject.KeyCode == Enum.KeyCode.ButtonA or inputObject.UserInputType == Enum.UserInputType.Touch then -- supports other input devices
        ProceedPurchase()
    end
end)

To allow other devices to interact with those UI objects.

1 Like
#10

What are the benefits of using InputBegan instead of MouseButton1Click?

#11

To my knowledge, MouseButton1Click will only work for PC devices (and maybe?) mobile devices, but not a gamepad controller. InputBegan gives you the option to check if the user is on mobile, PC or a controller so that you UI elements can be interacted with regardless of what device you’re on.

According to users in other posts, you can just use .Activated instead of what I posted.