Can anyone code review this, thank you

Help and Feedback Code Review
#1 
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)
1 Like
#2

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.

#3
  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
1 Like
#4

dam, loved this one, thanks for the info

1 Like