How could I make this script "better"

I don’t really no what I was doing , but I just put something together and tried something new, but I always have this thought that this can be done much better and more simple.

This is supposed to be a system where npcs keep spawning and I am very unsure about the loop I have for it. So right now it just waits a second and checks if the npc can spawn with “hascustomer” and waits how long the rate of the npcs is.

local CustomerService = coroutine.wrap(function(profile,PlotBuild,Plot,plr)
	while task.wait(1) do
		if profile.Data.Plot then

			for _, i in pairs(profile.Data.Plot) do
				for _, folders in pairs(PlotBuild:GetDescendants()) do
					if folders.Name == "CanvasObjects" and folders:IsA("Folder") then
						for _, buildings in pairs(folders:GetChildren()) do


							if buildings then
								if buildings.Name == i then
									if itemTypes[i] == "Stands" then
										local Stand = buildings
										if Stand and Stand.PrimaryPart then



											local CustomerSpawn = Plot:FindFirstChild("CustomerSpawn")
											if CustomerSpawn:FindFirstChild("Occupied").Value == false then
												local worker = Stand.PrimaryPart:FindFirstChild("Worker")
												print(worker)

												local hasCustomer = worker:FindFirstChild("HasCustomer")

												if hasCustomer.Value == false then
													hasCustomer.Value = true
													local Customer = Npcs.customer:GetChildren()[math.random(1,#Npcs.customer:GetChildren())]:Clone()
													Customer.Parent = Stand

													if Customer and Stand then
														Customer:PivotTo(CFrame.new(CustomerSpawn.Position))
														CustomerSpawn:FindFirstChild("Occupied").Value = true
														CustomerMOVE(Customer,Stand,plr,Plot)
														task.wait(profile.Data.CustomerRate)
														CustomerSpawn:FindFirstChild("Occupied").Value = false
													end
												end
											end
										end
									end
								end
							end
						end
					end
				end
			end	

		end


	end	
end)

Event.Event:Connect(function(plr)
	if plr then
	local Plot = Plots:FindFirstChild(plr:FindFirstChild("Plot").Value)
	local PlotBuild = Plot:FindFirstChild("Build")
	local profile = datastore.Profiles[plr]
	if profile.Data.Plot then
	
		CustomerService(profile,PlotBuild,Plot,plr)

I am basically looking for improvements and ideas from more experienced people than me.

3 Likes

should there be a thing that this coroutine should only run when the hascustomer is false , because I feel like the task wait could slow down the rate of the spawning since it waits a second to check

You provided a very small part of the script.

I am curious to see what I could improve about this part of the script

Sorry for not being clear

In my opinion there is a lot wrong with the code itself and the design of the code.

generally if you’re doing double back to back for loops there is a better way to optimize the solution (ie the most intuitive solution may be 2 for loops nested but there may exist a solution that only needs 1 for loop under the right conditions)

Also since your in a while loop, you might be doing things every iteration that you only need to do once.

I refactored a little of your code here


local CustomerService = coroutine.wrap(function(profile,PlotBuild,Plot,plr)
	while true do -- generally putting a function with a side effect as a conditional is a bad idea 
		task.wait(1);

		-- be descriptive with your variable namings and DO NOT NEST unless need be
		--[[
			ie

			if CONDITION then
				...
			end

			is WAY WORSE than

			if not CONDITION then
				return
			end

			when code is nested it way significantly harder to read.
		]]
		local hasPlot = profile.Data.Plot
		if not hasPlot then
			continue
		end

		--[[
			I put this in the while loop on the assumption things are being added and removed from the "PlotBuild"
			If this is not the case then it does not need to be in the while loop and can be put in front of the while loop!!!
			You are wasting computation if this does not need to be in the while loop
		]]
		local allBuildings = {}
		for _, folder in pairs(PlotBuild:GetDescendants()) do
			-- now we know these 2 conditions signify whether the object is valid or not
			--[[
				GOOD VARIABLE NAMING IS LIKE COMMENTING YOUR CODE; 
				if you name your variables properly and structure your code nicely it does not need to be commented
			]]
			local isValidFolder = folder.Name == "CanvasObjects" and folder:IsA("Folder")
			if not isValidFolder then
				continue
			end

			for _, buildings in pairs(folder:GetChildren()) do
				table.insert(allBuildings, buildings)
			end
		end

		for _, i in pairs(profile.Data.Plot) do
			for _, stand in pairs(allBuildings) do
				--[[
					"if buildings then"

					this if statement is USELESS! we already are grabbing non-nil objects we do not need to check if they exist or not
					this is called "domain knowledge". we **know** that this already exists so we do not need to check for it
				]]

				local isRelevant = stand.Name == i
				local isAStand = itemTypes[i] == "Stands"
				if not (isRelevant and isAStand) then
					continue
				end

				--[[
					again, we do not need to check if the stand exists or not because by looping through descendants,
					we know that it already exists
				]]

				-- a lot more readable than "if CustomerSpawn:FindFirstChild("Occupied").Value == false then"
				local CustomerSpawn = Plot:FindFirstChild("CustomerSpawn")
				local isOccupied = CustomerSpawn:FindFirstChild("Occupied").Value
				if not isOccupied then

					-- im going to stop here because im too lazy; but down below is also bad but same things mentioned above can fix it also.
					
					local worker = Stand.PrimaryPart:FindFirstChild("Worker")
					local hasCustomer = worker:FindFirstChild("HasCustomer")

					if hasCustomer.Value == false then
						hasCustomer.Value = true
						local Customer = Npcs.customer:GetChildren()[math.random(1,#Npcs.customer:GetChildren())]:Clone()
						Customer.Parent = Stand

						if Customer and Stand then
							Customer:PivotTo(CFrame.new(CustomerSpawn.Position))
							CustomerSpawn:FindFirstChild("Occupied").Value = true
							CustomerMOVE(Customer,Stand,plr,Plot)
							task.wait(profile.Data.CustomerRate)
							CustomerSpawn:FindFirstChild("Occupied").Value = false
						end
					end
				end
			end
		end
	end
end)

There is A LOT more, better, ways to redesign this code but these are the basics once you master the basics then you can start doing more advanced things (mainly with domain knowledge imo - like you can structure your data in a way where you only need 1 for loop, instead of 2 for loops nested, IF during a iteration there is no changes in “PlotBuild” but you would have check to ensure no changes etc, but that’s a lot of work to explain how to do and you desperately need to practice the basics first).

So overall;

  1. try not to nest if statements, loops, etc
  2. instead of if someObject.someProperty == spmeValue you should do proper variable naming and do
    something like
local isAStand = itemTypes[i] == "Stand"
if not isAStand then -- way more readable looking at this and if the condition is more complex I am essentially commenting on my code what this condition's purpose is (for this it may not be needed but regardless it will ALWAYS make your code better
    continue;
end
  1. Don’t put thing in while loops unless necessary; if I know something will stay stagnant and not change over the course of the while loop then no need to put that computation there!
  2. Also, for lua, people prefer to name variables camelCase like that instead of PascalCase like this (unless it is a service or custom object), also properties should be lowercased so according to lua conventions it should be profile.data.plot not profile.Data.Plot etc, and local stand = … instead of local Stand = …

There should never be that many nested logic branches, ever!!