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.
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
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;
try not to nest if statements, loops, etc
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
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!
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 = …