Is there anyway to optimize my script?

Help and Feedback Code Review
#1

Hi, so i wanted to ask is there anyway to optimize my code its around 68 lines and i dont like lengthy scripts so i want to optimize it, but i dont know how to start

Heres the script i use it in every buyable button:

local A = false

local Parent = script.Parent

local Item = game.ReplicatedStorage.Buyables.FlourBag

local Item2 = game.ReplicatedStorage.Buyables.Conveyor

local Button = game.ReplicatedStorage.Buyables.YeastButton

local Button2 = game.ReplicatedStorage.Buyables.WaterButton

local DropScript = Item.DropScript

local Fold = game.Workspace:WaitForChild("tycoon1")

local DropType = Fold.DropType

local FlourTransport = Item.Flour

local Price = 0

local Text = Parent["Price&Item"]["Price&ItemText"]

Text.Text = Item.Name.. ": " ..Price

local function Showbuttons()
	Button.Parent = Fold
	Button2.Parent = Fold
end

Parent.Touched:Connect(function(hit)
if A == false then
		A = true
	if hit.Parent:FindFirstChild("Humanoid") then
		local plr = game.Players:GetPlayerFromCharacter(hit.Parent)
		if plr then
			
			local cash = plr:WaitForChild("leaderstats").Coins
	
	if cash.Value >= Price then
			cash.Value -= Price

					print("bought!")
					
					Item.Parent = Fold
					
					Item2.Parent = Fold
					
					DropScript.Parent = Fold
					
					FlourTransport.Parent = Fold.Mixer
					
					DropType.Value = "FlourBall"
					
					Showbuttons()
					
					Parent.Parent:Destroy()
					
		elseif cash.Value < Price then
		print("To broke!")
		wait(2)
		A = false
				end
			end
		end
	end
end)
#2

Dont have blank lines in between variables.

1 Like
#3

that doesn’t help in optimization, infact it just makes your code look compact and maybe cleaner, depending on how you see it.

make game.ReplicatedStorage or game.ReplicatedStorage.Buyables a variable and use that instead of repetitively defining it like this. also use game:GetService("ReplicatedStorage") as it is better practice instead of directly defining them

this and any other wait() functions you have in your script should be task.wait. the task library is faster and more efficient

game.Players should be game:GetService("Players") in a separate variable

also, is this in a localscript? you should not be handling cash and price functions as it wont actually make a difference in the server (your actual cash value).

if this is a serverscript, *please don’t handle ui in the server. it can introduce performance issues.

#4

Mixed up on what OP needed, however still removing the empty lines would probably still make it readable.

Optimization wise, make variables for services instead of using them like: game.ReplicatedStorage....

2 Likes
#5

i tried refactoring it, this is first code review, im literally a beginner at scripting but i just wanted to post something so

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)

hope this works for you

#6

Referencing or retrieving a service like ReplicatedStorage will generally be more efficient and take less resources as you are using the same exact service you’ve already retrieved rather than retrieving the service every single time you wanna access something within it, so you should put it in a variable first like:

local replicated = game:GetService("ReplicatedStorage")

-- buyables
local buyables = replicated.Buyables
local flourBag = buyables.FlourBag;
local conveyor = buyables.Conveyor

-- buttons
local yeastBtn = buyables.YeastButton;
local waterBtn = buyables.WaterButton
-- and so on...

It’s also best to give actual meaning to the name of your variables. When you look at the name of a variable, it should be meaningful—you should be able to somewhat tell what it is alone, without even looking at the value itself. “Item” or “Button” don’t really give a lot of information, but waterBtn or flourBag do. You made this exact mistake for your boolean variable named “A”, which I’m not even sure what that’s supposed to do, but all of these are less about optimisations, more about just writing clean code in general with the help of programming naming conventions.

Likewise, like what others said, you also completely avoid unnecessary blank lines in-between variables—not only are those unnecessary additions of lines to your code, they also are just not very pleasant to the eyes at all. When you write an essay, you don’t implement a blank line in-between each sentence; you only do so if it’s a new paragraph. Writing code is similar to writing essays in its own way (at least in my opinion).

Additionally, rather than creating a separate if-block for your Variable A, you could just do a short-form if-else statement, like so:

A = (A == false and true or false);
-- if A is equal to false then set it to true, otherwise set it to false

And if you want your block of code within the event to run only if A is equal to false, then:

if A ~= false then return; end;

Basically, if A is not equal to false, then it will return and will therefore not reach the code below. Make sure not to forget the semi-colons, as they are necessary when writing multiple statements in a line (as seen above). They’re statement separators, and the above is basically equal to

if A ~= false then
    return
end

But with the semi-colon, it allows you to compile it into one line.