How can I optimize my code

Help and Feedback Scripting Support
#1

Hey, Im a newer scripter trying to make my code optimized and organized. Below is a snippet of code I wrote for structure generation, what are some mistakes I made and some things I can work on.

local rs = game:GetService("ReplicatedStorage")

--settings
local distance = 250
local variance = 70
local amountperring = 20
local mindistance = 150
local maxdistance = 2000
local maxdistfromplayer = 2000

local segments = rs.Segments

local segmentsfolder = game.Workspace.Structures
local enemiesfolder = game.Workspace.Enemies

local spawns = {Vector3.new(0, 0, 0)}

local function generaterandompos()
	local lv = (Vector3.new(math.random(-100, 100), 0, math.random(-100, 100)) / 100).Unit
	local amp = distance + math.random(-variance, variance)

	local final = lv * amp
	return final
end

local function findspawnlocation(playerpos)
	local position
	while not position do
		local potentialpos = generaterandompos()
		local closestdist = 10000

		for i, v in pairs(spawns) do
			local distance = (potentialpos - v).Magnitude
			if distance < closestdist then
				closestdist = distance
			end
		end
		local nearestplayer
		local nearestplayerdist = maxdistfromplayer

		local chars = game.Workspace:GetChildren()
		for i, v in ipairs(chars) do
			local hum = v:FindFirstChildOfClass("Humanoid")
			if hum then
				local hrp = v.HumanoidRootPart

				local distance = (potentialpos - hrp.Position).Magnitude
				if distance < nearestplayerdist then
					nearestplayer = v
					nearestplayerdist = distance
				end
			end
		end

		if closestdist > mindistance and closestdist < maxdistance and nearestplayer then
			position = potentialpos + playerpos
		end
		task.wait()
	end


	local rotation = Vector3.new(0, math.rad(math.random(-180, 180)), 0)

	local cframe = CFrame.new(position, rotation)
	return cframe
end

local function choosestructure(pos)
	local level = math.round(pos.Magnitude/700)
	local clampedlevel = math.clamp(level, 1 , 4)

	local levelsegments = segments:FindFirstChild("level"..clampedlevel)

	local segment = levelsegments:GetChildren()[math.random(1, #levelsegments:GetChildren())]
	return segment
end

local function loadstructure(structure, cframe)
	local newstruct = structure:Clone()

	task.wait()
	newstruct.Parent = segmentsfolder
	task.wait()
	newstruct:PivotTo(cframe)
	task.wait()

	local structureenemies = newstruct.Enemies
	local enemies = structureenemies:GetChildren()
	for i, v in ipairs(enemies) do
		v.Parent = enemiesfolder
	end

	structureenemies:Destroy()
end


local function generatestructure(playerpos)
	local spawncframe = findspawnlocation(playerpos)
	local segment = choosestructure(spawncframe.Position)
	table.insert(spawns, spawncframe.Position)

	loadstructure(segment, spawncframe)
end

while true do
	for i, v in ipairs(game.Players:GetChildren()) do
		local char = v.Character
		if char then
			print("j")
			local hrp = char.HumanoidRootPart
			generatestructure(hrp.Position)
		end
		task.wait()
	end

	task.wait(1)
end
#2

Honestly, for a “new scripter”, this is pretty solid. Just looking at this face level without diving into the logic or context, here are my suggestions if you want to step up your code. First, you should read this. It’s a bit long, but its helpful when working on teams to make sure code feels consistent. Also, when creating a for loop, unless it’s like 1 or 2 lines of content inside, try and name the variable something more accurate than v (such as “enemy”). When going back into old code or other people’s code, it’s helpful to know what “v” is at a glance without having to backtrack to the top of the for loop. Don’t use “i” in a for loop unless you need it; use “_”. Finally, when you name service names, it’s recommended to use the full service name and not an abbreviation. This is because a lot of people use require autocomplete.

1 Like
#3

Since the person above provided organization, I’m gonna provide some optimization tips:

  1. Native Code Generation
    The easiest way to make your script faster, but also comes with trade-offs. More memory footprint, somewhat slower to compile, and limited uses (you can’t use native code gen for all scripts). One way to do this is putting --!native at the top of your code:
--!native
-- your code...
  1. Type Annonations
    This can also make your code look nicer. Type annotations will help your intellisense infers variable types and make suggestions based on that. Type annotations can also boost the performance if native code generation is active. Type annotations defines what a variable should be:
local position: Vector3

It also can be used in function parameters:

local function chooseStructure(pos: Vector3)
-- ...
end

Though you shouldn’t annotate it if the intellisense shows you the type without any annotations:

local distance: number = 250 -- unnecessary number annotation since it already knows its a number
1 Like
#4

You can replace your Vector3s with native vectors instead to improve performance, since you are constructing vectors often.

Use the workspace global, as the Engine will have to call a function to figure out what game.Workspace is equal to each time you use it.

You should declare your services at the top of your script, similar reason in terms of performance as above, I’m not sure why you’ve done this for ReplicatedStorage but not Players (also, you should use better variable names than rs, but that’s more stylistic).

The majority of the time in pairs(t) and in ipairs(t) can be replaced by in t directly, i.e.:

for i, v in spawns do

This behaves identically to pairs. You do not need to use ipairs 99.99% of the time unless you only care about iterating through the array part of a mixed table (i.e. only indexes counting up from 1 in a table with number indexes and keys of other types, but that is not present in your code). This should remove the function call to pairs/ipairs.

You could use String Interpolation here:

-- String Interpolation needs backticks (`), NOT quotes
`level{clampedLevel}`

, but I’m not sure if it would be any faster.

You use :FindFirstChild, but don’t seem to care if it returns nil by calling :GetChildren directly after. :FindFirstChild is a lot slower than a direct index, so use that instead:

This can be extremely inefficient if you have many Instances under the Workspace. Iterate through players directly and get their character from that instead

for _, player in Players:GetPlayers()
    local v = player.Character
    local hum = if v then v:FindFirstChild("Humanoid") else nil
    if hum then

Why are you doing this?

#5

Thank you this is all great advice and I will implement this but the reason I used the task.wait() s is because each one of those lines causes about 15 ms to do so if i put them in 1 thing, it will take 40-60ms and combined with everything else the total comes to be about 100ms which is not ideal

#6

They shouldn’t be taking so long, it might be a bug with Roblox. Do you have any MeshParts in your game?