Can anyone code review this, thank you

[mattilpea]

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

local Brick = script.Parent
local Buyables = ReplicatedStorage.Buyables
local Item = Buyables.FlourBag
local holderTable = {Buyables.FlourBag, Buyables.Conveyor,  Buyables.YeastButton, Buyables.WaterButton, Item.DropScript }

local Fold = workspace:WaitForChild("tycoon1", 5)
local DropType = Fold.DropType
local FlourTransport = Item.Flour

local Price = 0
local DebounceTime = 2
local PriceAndItemText = Brick["Price&Item"]["Price&ItemText"] --correct me if im wrong but is it a textlabel ? if so, write something more descriptive
PriceAndItemText.Text = `{Item.Name} : ${Price}` 

local function LoopAndParent(t: {[string]: Instance} , parentInstance : Instance  )
	for _, item in pairs(t) do
		item.Parent = parentInstance
	end
end

local Debounce = false
local function brickTouchedEvent(hit : BasePart )
	if Debounce == false then 
		Debounce = true
		if hit.Parent:FindFirstChild("Humanoid") then
			if Players:GetPlayerFromCharacter(hit.Parent) then
				local char = hit.Parent
				local plr = Players:GetPlayerFromCharacter(char)
				local Coins = plr:WaitForChild("leaderstats", 5).Coins
				if Coins.Value >= Price then
					Coins.Value -= Price
					print("bought!") ---write a debugger function or something to be able to verbose log these
					LoopAndParent(holderTable, Fold )
					FlourTransport.Parent = Fold.Mixer
					DropType.Value = "FlourBall"
					Brick.Parent:Destroy()
				elseif Coins.Value < Price then
					print("Too broke!")
					task.wait(DebounceTime)
					Debounce = false
				end
			end
		end
	end
end

Brick.Touched:Connect(brickTouchedEvent)

[Glitchifyed]

I’m not entirely sure what you’re looking for, but I guess I do have a few things of note.

If you’re checking if value >= value2 then, you don’t then need to perform an elseif check to see if it’s fewer, as that would be a given anyway. It might help for code readability.

Another good idea would be to use more line spacing. Try spacing out different ‘actions’ as I’d normally call them.

You also don’t actually need to check if a boolean is equal to another boolean. Just doing if bool then also works.

If you are looking to make your code inside a function a little more readable, you could also try to avoid nesting whenever you can. Instead of doing if Debounce == false then and wrapping the next code in the line inside that if statement, you could try doing: if Debounce then return end and put the rest of the code underneath that. I find it helps me to read my code easier.

[sonic_848 (killer)]

1. To be honest, you shouldn’t really be using WaitForChild for the leader stats folder because it’s already created the instant the player joins so it’s either they have it they not. Just use FindFirstChild for things that you know exist… (Kind of ambiguous)
2. Your code gets a little repetitive when you use the Players:GetPlayerFromCharacter twice

Other than these light suggestions, I don’t have much to say. I edited the function a little bit to show you to show you what I was talking about

local function brickTouchedEvent(hit : BasePart )
	if Debounce then return end
	Debounce = true
	
	local Player = Players:GetPlayerFromCharacter(hit.Parent)
	if not Player then return end
	
	local Leaderstats = Player:FindFirstChild("leaderstats")
	if not Leaderstats then return end
	
	local Coins = Leaderstats:FindFirstChild("Coins") 
	if not Coins then return end
	
	local CanBuy = (Coins.Value >= Price) and true or false --This is basically a condensed if statement
	
	if CanBuy then
		Coins.Value -= Price
		print("bought!") ---write a debugger function or something to be able to verbose log these
		LoopAndParent(holderTable, Fold )
		FlourTransport.Parent = Fold.Mixer
		DropType.Value = "FlourBall"
		Brick.Parent:Destroy()
	else	
		print("Too broke!")
		task.wait(DebounceTime)
		Debounce = false
	end
end

[mattilpea]

dam, loved this one, thanks for the info

[sports_beast2]

I don’t believe this is true; the leaderstats folder is being created by some other script, meaning it isn’t necessarily created on join. Yes, the player joins, it gets created, but if this script runs before the folder is created, then it will error, so WaitForChild would be the correct practice.

[sonic_848 (killer)]

This is a touched event meaning the player would’ve long had their leaderstats folder before they triggered the event.

[sports_beast2]

Still, just to be safe. Hypothetically, if the player were to spawn on the object, the touched event would trigger, but the leaderstats folder may not have loaded in. Cases like these must be accounted for, so WaitForChild would prevent such an error.

[sonic_848 (killer)]

What’re you talking about
Even if that did happen it’s not like we need to 100℅ detect if the player stepped on it. The player could always retry. Also, your leaderstats will most like spawn way before your character fully loads into the game

[sports_beast2]

You can’t rely on “most likely” to suffice. One small network disruption can cause leaderstats to load in half a second later, for instance. If a script proceeds under the assumption that leaderstats is always instantly available, it can cause errors when trying to access non-existent values.

[sonic_848 (killer)]

@sports_beast2

If a script proceeds under the assumption that leaderstats is always instantly available, it can cause errors when trying to access non-existent values.

Did you read the code I edited? The code will NEVER access non-existent values or maybe you can point out the points of possible errors?