Shop script feedback

Help and Feedback Code Review
1

So I made a shop and it looks good. How can I make it better?


Client

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Camera = workspace.CurrentCamera
local TweenService = game:GetService("TweenService")
local pastPosition = script.Parent.ShopFrame.Position

ReplicatedStorage.Events.ShopOpen.OnClientEvent:Connect(function(shop,info,prompt)
	prompt.Enabled = false
	local ogPos = script.Parent.ShopFrame.Icon.Position
	local pastCFrame = Camera.CFrame
	local pastSize = script.Parent.ShopFrame.Icon.Size
	local index = 0
	local closed = false
	Camera.CameraType = Enum.CameraType.Scriptable
	script.Parent.ShopFrame.Position = pastPosition - UDim2.new(1,0,0,0)
	local MusicFadeOutTween = TweenService:Create(game.SoundService.Music.MainMusic, TweenInfo.new(0.5, Enum.EasingStyle.Linear, Enum.EasingDirection.Out), {Volume = 0})
	local MusicFadeInTween = TweenService:Create(game.SoundService.Music.MainMusic, TweenInfo.new(0.5, Enum.EasingStyle.Linear, Enum.EasingDirection.Out), {Volume = 1})
	local EnterFrameTween = TweenService:Create(script.Parent.ShopFrame, TweenInfo.new(0.5, Enum.EasingStyle.Cubic, Enum.EasingDirection.Out), {Position = pastPosition})
	local ExitFrameTween = TweenService:Create(script.Parent.ShopFrame, TweenInfo.new(0.5, Enum.EasingStyle.Cubic, Enum.EasingDirection.Out), {Position = pastPosition + UDim2.new(1,0,0,0)})
	local EnterTween = TweenService:Create(Camera, TweenInfo.new(0.5, Enum.EasingStyle.Cubic, Enum.EasingDirection.Out), {CFrame = shop.CameraPos.CFrame})
	local ExitTween = TweenService:Create(Camera, TweenInfo.new(0.5, Enum.EasingStyle.Cubic, Enum.EasingDirection.Out), {CFrame = pastCFrame})
	EnterTween:Play()
	print(info)
	game.SoundService.Music.ShopMusic:Play()
	local function playSound(soundId)
		local sound = Instance.new("Sound")
		sound.Parent = game.SoundService.NewSoundFolder
		sound.SoundId = soundId
		sound.Volume = 1
		sound:Play()
		sound.Loaded:Wait()
		task.delay(sound.TimeLength + 0.05, function()
			if sound then sound:Destroy() end
		end)
	end
	local function randomQuote()
		script.Parent.ShopFrame.Icon.Position = ogPos + UDim2.new(0,0,0,-20)
		local talkTween = TweenService:Create(script.Parent.ShopFrame.Icon, TweenInfo.new(0.25, Enum.EasingStyle.Cubic, Enum.EasingDirection.Out), {Position = ogPos})
		local quotes = info["Quotes"]
		local random = math.random(1, #quotes)
		local soundId = info["TalkSound"]
		talkTween:Play()
		script.Parent.ShopFrame.Icon.QuoteText.Text = quotes[random]
		task.spawn(playSound, soundId)
	end
	local function randomBuyQuote()
		script.Parent.ShopFrame.Icon.Position = ogPos + UDim2.new(0,0,0,-20)
		local talkTween = TweenService:Create(script.Parent.ShopFrame.Icon, TweenInfo.new(0.25, Enum.EasingStyle.Cubic, Enum.EasingDirection.Out), {Position = ogPos})
		local quotes = info["SuccessBuyQuotes"]
		local random = math.random(1, #quotes)
		local soundId = info["TalkSound"]
		talkTween:Play()
		script.Parent.ShopFrame.Icon.QuoteText.Text = quotes[random]
		task.spawn(playSound, soundId)
	end
	local function randomFailBuyQuote()
		script.Parent.ShopFrame.Icon.Position = ogPos + UDim2.new(0,0,0,-20)
		local talkTween = TweenService:Create(script.Parent.ShopFrame.Icon, TweenInfo.new(0.25, Enum.EasingStyle.Cubic, Enum.EasingDirection.Out), {Position = ogPos})
		local quotes = info["FailBuyQuotes"]
		local random = math.random(1, #quotes)
		local soundId = info["TalkSound"]
		talkTween:Play()
		script.Parent.ShopFrame.Icon.QuoteText.Text = quotes[random]
		task.spawn(playSound, soundId)
	end
	local function randomSquishQuote()
		local quotes = info["SquishQuotes"]
		local random = math.random(1, #quotes)
		local soundId = info["TalkSound"]
		task.spawn(playSound, soundId)
		script.Parent.ShopFrame.Icon.QuoteText.Text = quotes[random]
	end
	local function setupIcon()
		script.Parent.ShopFrame.Icon.Image = info["Icon"]
		randomQuote()
		local connection = script.Parent.ShopFrame.Icon.MouseButton1Click:Connect(function()
			script.Parent.ShopFrame.Icon.Size = pastSize + UDim2.new(0,50,0,-50)
			local squishTween = TweenService:Create(script.Parent.ShopFrame.Icon, TweenInfo.new(0.3, Enum.EasingStyle.Cubic, Enum.EasingDirection.Out), {Size = pastSize})
			local soundId = game.SoundService.SFX.Squish.SoundId
			squishTween:Play()
			randomSquishQuote()
			print("press")
			task.spawn(playSound, soundId)
		end)
		task.spawn(function()
			while task.wait() do
				if closed then connection:Disconnect() break end
			end
		end)
	end
	script.Parent.Enabled = true
	setupIcon()
	MusicFadeOutTween:Play()
	EnterFrameTween:Play()
	for _, v in ipairs(script.Parent:GetDescendants()) do
		if v:IsA("ImageButton") and v:HasTag("ShopButton") then
			index += 1
			local itemIndex = index

			local itemInfo = info["Items"][itemIndex]
			local price = itemInfo.Price
			local itemFrame = script.Parent.ShopFrame["Item"..itemIndex]
			itemFrame.Image = itemInfo.Icon
			itemFrame["Item"..itemIndex.."Label"].Text = itemInfo.Name.. "   $".. price
			
			local connection2 = v.MouseButton1Click:Connect(function()
				if game.Players.LocalPlayer.leaderstats.Cash.Value >= price then
					randomBuyQuote()
					task.spawn(playSound, game.SoundService.SFX.BuyItem.SoundId)
					ReplicatedStorage.Events.BuyItem:FireServer(itemInfo.ItemName, price)
				else
					randomFailBuyQuote()
				end
			end)
			
			task.spawn(function()
				while task.wait() do
					if closed then
						connection2:Disconnect()
						break
					end
				end
			end)
		end
	end
	local connection
	
	connection = script.Parent.ShopFrame.CloseButton.MouseButton1Click:Connect(function()
		ExitTween:Play()
		connection:Disconnect()
		closed = true
		game.SoundService.Music.ShopMusic:Stop()
		game.SoundService.SFX.Bye:Play()
		ExitFrameTween:Play()
		ExitTween.Completed:Wait()
		MusicFadeInTween:Play()
		script.Parent.Enabled = false
		prompt.Enabled = true
		Camera.CameraType = Enum.CameraType.Custom
	end)
end)

Server

local Shops = workspace:WaitForChild("Shops")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

for i,shop in Shops:GetDescendants() do
	if shop:IsA("Model") then
		if shop:HasTag("Shop") then
			for i,v in shop:GetDescendants() do
				if v:IsA("BasePart") then
					if v.Name == "ShopPrompt" then
						local ProximityPrompt = Instance.new("ProximityPrompt")
						local ShopInfo = v:WaitForChild("ShopInfo")
						ProximityPrompt.Parent = v
						ProximityPrompt.ActionText = "Open"
						ProximityPrompt.ObjectText = "Shop"
						ProximityPrompt.Triggered:Connect(function(player)
							ReplicatedStorage.Events.ShopOpen:FireClient(player, shop, require(ShopInfo), ProximityPrompt)
						end)
					end
				end
			end
		end
	end
end

Module

local shopinfo = {
	["Items"] = {
		[1] = {
			Name = "Teddy Bear",
			ItemName = "Teddy",
			Icon = "rbxassetid://8073107221",
			Price = 20
		},
		[2] = {
			Name = "Teddy Bear",
			ItemName = "Teddy",
			Icon = "rbxassetid://8073107221",
			Price = 20
		},
		[3] = {
			Name = "Teddy Bear",
			ItemName = "Teddy",
			Icon = "rbxassetid://8073107221",
			Price = 20
		},
		[4] = {
			Name = "Teddy Bear",
			ItemName = "Teddy",
			Icon = "rbxassetid://8073107221",
			Price = 20
		},
		[5] = {
			Name = "Teddy Bear",
			ItemName = "Teddy",
			Icon = "rbxassetid://8073107221",
			Price = 20
		},
		[6] = {
			Name = "...",
			ItemName = "AK47",
			Icon = "rbxassetid://9822883942",
			Price = 20
		},
	},
	["Icon"] = "rbxassetid://115838236744256",
	["TalkSound"] = "rbxassetid://5417004822",
	["Quotes"] = {
		"hi",
		"yo whats up",
		"eeeeeeeee"
	},
	["SuccessBuyQuotes"] = {
		"yoo thanks",
		"thank you!",
		"ur the best"
	},
	["FailBuyQuotes"] = {
		"oh you cant afford that lol",
		"bruh when will i get the money",
		"aw man"
	},
	["SquishQuotes"] = {
		"ow",
		"stop",
		"ah",
		"what"
	}
}

return shopinfo

Don’t mind the unnecessary quotes, items, and music. The game was just a test.

2 Likes
2

Check if the player has enough money again on the server to prevent exploiters from purchasing items when they don’t have enough cash

3 Likes
3

Can do that.

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ServerStorage = game:GetService("ServerStorage")

ReplicatedStorage.Events.BuyItem.OnServerEvent:Connect(function(player, itemName, price)
	local toolsFolder = game.ReplicatedStorage.Items
	local tool = toolsFolder:FindFirstChild(itemName)

	if tool and player.leaderstats.Cash.Value >= price then
		player.leaderstats.Cash.Value -= price
		local clone = tool:Clone()
		clone.Parent = player.Backpack
	end
end)
4

Exploiters can send in an altered price in the parameters
For fix, you can make the client send exclusively the INDEX of the purchase they are doing, and the server gets the price and item by itself

4 Likes
5

Okay, so I think this works. I added this to my local script.

connection2 = ReplicatedStorage.Events.ShopBuyResult.OnClientEvent:Connect(function(isEnough)
		if isEnough == true then
			randomBuyQuote()
			task.spawn(playSound, game.SoundService.SFX.BuyItem.SoundId)
		else
			randomFailBuyQuote()
		end
	end)

I changed my server script too.

local ReplicatedStorage = game:GetService("ReplicatedStorage")

local BuyEvent = ReplicatedStorage.Events:WaitForChild("BuyItem")

local function getCashStat(player)
	local stats = player:FindFirstChild("leaderstats")
	if not stats then return end
	return stats:FindFirstChild("Cash")
end

BuyEvent.OnServerEvent:Connect(function(player, shop, index)
	print(shop)
	print(index)
	if typeof(shop) ~= "Instance" or type(index) ~= "number" then return end
	if not shop:IsDescendantOf(workspace.Shops) then return end
	
	local promptPart = shop:FindFirstChild("ShopPrompt", true)
	if not promptPart then return end

	local shopInfoModule = promptPart:FindFirstChild("ShopInfo")
	if not shopInfoModule or not shopInfoModule:IsA("ModuleScript") then return end

	local shopInfo = require(shopInfoModule)
	local itemData = shopInfo.Items[index]
	if not itemData then return end

	local cash = getCashStat(player)
	if not cash then return end

	local price = itemData.Price
	if cash.Value < price then
		ReplicatedStorage.Events.ShopBuyResult:FireClient(player, false)
		return
	end

	
	cash.Value -= price

	
	local toolTemplate = ReplicatedStorage.Items:FindFirstChild(itemData.ItemName)
	if toolTemplate then
		local tool = toolTemplate:Clone()
		tool.Parent = player.Backpack
	end
	
	ReplicatedStorage.Events.ShopBuyResult:FireClient(player, true, itemData.Name)
end)
3 Likes
6

That is much better and more secure, great job

1 Like
7

— Scripting —
Well I mean you could make trigger menu UI based on client

you could put those functions outside of the function OnClientEvent (in ModuleScript you could store those functions) (this does requires rewriting code or not, depending on your own code)

you could store up those locals outside of function
— Security —

Personally you could add like if player is required position to able to buy items (Anti-cheat)

like make Raycast or use part as hitbox and player is in that hitbox then make Event Buy() successfully

— Extra —
and probably make clean(er) code ? (depends on your choice)

8

I swear I don’t understand what’s going on.. but you did it pretty decent.

This reminds me the time I got to solve the same problems.

1 Like
9

You might want to have shop information replicated to the client on join (as a ModuleScript, StringValue, or through a RemoteEvent message) to avoid sending the same data every time the shop is opened. Give each shop an ID and simply refer to the shop information based on that ID. Dynamic shops wouldn’t be a problem, either, as you could just code a certain ID/range/flag to act differently.

Just disconnect it when you close. You have more code and less efficiency to no benefit.

local connection1
...
local function setupIcon()
	connection1 = script.Parent.ShopFrame.Icon.MouseButton1Click:Connect( ...
	...
end
...
connection = script.Parent.ShopFrame.CloseButton.MouseButton1Click:Connect(function()
		...
		if connection1 then connection1:Disconnect() end

If you find naming every connection untenable, use an array.

connections = {}
...
connections[#connections + 1] = ...
...
for _, conn in connections do
	conn:Disconnect()
end
1 Like
10

i would make it so there is more of a notification for actually buying something, along with needing a confirm button so somebody doesn’t midclick, maybe blur background if you feel like it. i don’t really know the style you’re going for here so that’s just the basic improvements i would make.

11

This is gonna highlight only some problems since, i don’t really wanna spend much time

So let’s start

You are doing all that just to check the tag at the end, you can just use CollectionService:GetTagged.

Why? Listening for prompts on the client would be much easier, you can always just tag them something like ShopPrompt, and use collection service.

You are repeating yourself, you can do something like

local function randomQuote(name: string)

and then simply access it by

local quotes = info[name]

Just add it to a table, like connections and disconnect it a the end, also doing

while task.wait() do
-- is pointless since, you actually want to
while not closed do

i hope i could help atleast a bit, there is way more to this script that could be improved, i just don’t have the time and energy to search.

4 Likes
12

this is much better code than the initial post; just know in the future when using conditionals never nest this far

your code 
local Shops = workspace:WaitForChild("Shops")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

for i,shop in Shops:GetDescendants() do
	if shop:IsA("Model") then
		if shop:HasTag("Shop") then
			for i,v in shop:GetDescendants() do
				if v:IsA("BasePart") then
					if v.Name == "ShopPrompt" then
						local ProximityPrompt = Instance.new("ProximityPrompt")
						local ShopInfo = v:WaitForChild("ShopInfo")
						ProximityPrompt.Parent = v
						ProximityPrompt.ActionText = "Open"
						ProximityPrompt.ObjectText = "Shop"
						ProximityPrompt.Triggered:Connect(function(player)
							ReplicatedStorage.Events.ShopOpen:FireClient(player, shop, require(ShopInfo), ProximityPrompt)
						end)
					end
				end
			end
		end
	end
end

sometimes you can put a few conditionals together; or outright make one line guard clauses to filter out any checks that would return false
look into “never nesting” as an example, esp. guard clauses

my code

for _, shop in ipairs(Shops:GetDescendants()) do
if not (shop:IsA(“Model”) and shop:HasTag(“Shop”)) then continue end

for _, v in ipairs(shop:GetDescendants()) do
	if v:IsA("BasePart") and v.Name == "ShopPrompt" then
		local prompt = Instance.new("ProximityPrompt")
		local ShopInfo = v:WaitForChild("ShopInfo")

		prompt.Parent = v
		prompt.ActionText = "Open"
		prompt.ObjectText = "Shop"

		prompt.Triggered:Connect(function(player)
			ReplicatedStorage.Events.ShopOpen:FireClient(player, shop, require(ShopInfo), ProximityPrompt)
		end)
	end
end

end

^ hopefully this works