How can I improve this code?

Help and Feedback Code Review
, , , ,
#1

Heya there.

I’m currently rewriting an OOP code for a fangame I’m working on called “Noobs vs Zombies: Relish Reborn”. I’ve did notice some issues with the code however and I’m wondering how I can fix them. The issues I’ve seen so far are:

  • NPC’s not switching targets whenever they’re near an enemy.
  • NPC’s getting ‘stuck’ whenever an enemy dies where they’ll walk back and fourth
  • The code is poorly formatted.

The code is still in WIP, though criticism should be helpful to make it more readable in the future.

--[[SERVICES]]--
local Players = game:GetService("Players")
local PathfindingService = game:GetService("PathfindingService")

--[[NPC MODEL FOLDERS]]--
local NPCFolder = game:GetService("ServerScriptService").NPCS
local NoobsFolder = NPCFolder.Noobs
local ZombiesFolder = NPCFolder.Zombies

--[[MODULE]]--
local BaseNPCModule = {}
BaseNPCModule.__index = BaseNPCModule

function BaseNPCModule:CreateNPC(UnitName, UnitTeam, Weapon, Special)
	--//Checking to see if the unit exists.
	--//Also checking what team the unit is on as well
	if UnitTeam == "Noobs" then
		
		for _, FindUnit in pairs(NoobsFolder:GetChildren()) do
			if FindUnit:IsA("Model") and FindUnit.Name == UnitName then

				local self = setmetatable({}, BaseNPCModule)

				--//Creating the unit.
				self.Unit = FindUnit:Clone()
				self.Humanoid = self.Unit:FindFirstChildWhichIsA("Humanoid")
				self.Unit.Parent = game.Workspace
				self.UnitTeam = game:GetService("CollectionService"):AddTag(self.Unit, UnitTeam)
				self.Is_Special = Special

				--//Spawning it to the map.
				local Noob_Spawns = workspace:FindFirstChild("NoobSpawns")
				local SelectedSpawns = Noob_Spawns:GetChildren()
				local Pos = math.random(1,#SelectedSpawns)
				local SpawnPosition = SelectedSpawns[Pos]
				
				self.Unit.PrimaryPart.CFrame = SpawnPosition.CFrame + Vector3.new(0, 3, 0)

				--//Giving it functionality
				if not self.Is_Special then
					--//Assuming it's just a basic NPC (E.g. Noob/Zombie Sworder)
					--//Otherwise, the NPC is going to inheritate only the FindNearestTargets and Pathfind functions.
					task.spawn(function()
						while task.wait(0.05) do
							task.spawn(function()
								local Target = self:FindNearestTargets()

								if Target then
									self:Pathfind(Target)
								end
							end)
						end
					end)

				end

				return self

			end
		end
		
	elseif UnitTeam == "Zombies" then
		
		for _, FindUnit in pairs(ZombiesFolder:GetChildren()) do
			if FindUnit:IsA("Model") and FindUnit.Name == UnitName then

				local self = setmetatable({}, BaseNPCModule)

				--//Creating the unit.
				self.Unit = FindUnit:Clone()
				self.Humanoid = self.Unit:FindFirstChildWhichIsA("Humanoid")
				self.Unit.Parent = game.Workspace
				self.UnitTeam = game:GetService("CollectionService"):AddTag(self.Unit, UnitTeam)
				self.Is_Special = Special

				--//Spawning it to the map.
				local Zombie_Spawns = workspace:FindFirstChild("ZombieSpawns")
				local SelectedSpawns = Zombie_Spawns:GetChildren()
				local Pos = math.random(1,#SelectedSpawns)
				local SpawnPosition = SelectedSpawns[Pos]

				self.Unit.PrimaryPart.CFrame = SpawnPosition.CFrame + Vector3.new(0, 3, 0)
				--//Giving it functionality
				if not self.Is_Special then
					--//Assuming it's just a basic NPC (E.g. Noob/Zombie Sworder)
					--//Otherwise, the NPC is going to inheritate only the FindNearestTargets and Pathfind functions.
					task.spawn(function()
						while task.wait(0.05) do
							task.spawn(function()
								local Target = self:FindNearestTargets()

								if Target then
									self:Pathfind(Target)
								end
							end)
						end
					end)

				end

				return self

			end
		end
		
	elseif UnitTeam == "Neutral" then
		--//TBA
	end
end

function BaseNPCModule:FindNearestTargets()
	local Targets = {}
	local Nearest = math.huge
	local Target = nil

	for _, PossibleTargets in ipairs(workspace:GetChildren()) do
		if PossibleTargets:IsA("Model") and PossibleTargets:FindFirstChildWhichIsA("Humanoid") then
			local Humanoid = PossibleTargets:FindFirstChildWhichIsA("Humanoid")
			if Humanoid.Health <= 1 then
				--//They're dead.
				return
			end

			--//If they aren't dead, check the distance.
			local Distance = (PossibleTargets.PrimaryPart.Position - self.Unit.PrimaryPart.Position).Magnitude
			if Distance <= Nearest then

				if PossibleTargets:HasTag("Zombies") and self.Unit:HasTag("Noobs") then
					table.insert(Targets,{
						Magnitude = Nearest,
						FoundTarget = PossibleTargets
					})
				elseif PossibleTargets:HasTag("Noobs") and self.Unit:HasTag("Zombies") then
					table.insert(Targets,{
						Magnitude = Nearest,
						FoundTarget = PossibleTargets
					})
				end

			end
		end
	end

	for _, Entry in pairs(Targets) do
		local magnitude = Entry.Magnitude
		local target = Entry.FoundTarget

		if magnitude <= Nearest then
			Nearest = magnitude
			Target = target
		end
	end

	return Target
end

function BaseNPCModule:Pathfind(Target)
	local NewPath = PathfindingService:CreatePath({
		AgentRadius = 3,
		AgentHeight = 5,
		AgentCanJump = true,
	})
	
	NewPath:ComputeAsync(self.Unit.PrimaryPart.Position, Target.PrimaryPart.Position)
	if NewPath.Status == Enum.PathStatus.Success then
		local Waypoints = NewPath:GetWaypoints()

		for i, Waypoint in ipairs(Waypoints) do
			if i <= 2 then continue end
			if Waypoints[4] then
				self.Humanoid:MoveTo(Waypoints[4].Position)

				local TimeOut = self.Humanoid.MoveToFinished:Wait()
				if not TimeOut then
					warn("NPC got stuck... Recalculating!")
					BaseNPCModule:Pathfind(Target)
				end
			end
		end
	end
end

return BaseNPCModule
2 Likes
#2

note: i’m not going to go over the functionality
really the only issue i see is the heavy indentation (9 tabs deep is ridiculous) and the potentially long elseif chains

move the UnitTeam blocks into their own functions then work on the if statements.

it seems like you Know what guard clauses are, but 50% of the time you just don’t make them.
let’s just look at this loop (i’d usually clean the whole script but i don’t feel like it)

for i, Waypoint in ipairs(Waypoints) do
  if i <= 2 then continue end -- This is good
  if Waypoints[4] then -- But why didn't you do the same here?
    self.Humanoid:MoveTo(Waypoints[4].Position)

    local TimeOut = self.Humanoid.MoveToFinished:Wait()
    if not TimeOut then
      warn("NPC got stuck... Recalculating!")
      BaseNPCModule:Pathfind(Target)
    end
  end
end

for i, Waypoint in Waypoints do
  if i <= 2 then continue end

  local TargetWaypoint = Waypoints[4]
  if not TargetWaypoint then continue end

  self.Humanoid:MoveTo(TargetWaypoint.Position)

  local MovedToTarget = self.Humanoid.MoveToFinished:Wait()
  if MovedToTarget then continue end

  warn("NPC got stuck... Recalculating!")
  BaseNPCModule:Pathfind(Target)
end

also, what’s the difference here?

if PossibleTargets:HasTag("Zombies") and self.Unit:HasTag("Noobs") then
  table.insert(Targets,{
    Magnitude = Nearest,
    FoundTarget = PossibleTargets
  })
elseif PossibleTargets:HasTag("Noobs") and self.Unit:HasTag("Zombies") then
  table.insert(Targets,{
    Magnitude = Nearest,
    FoundTarget = PossibleTargets
  })
end

if (PossibleTargets:HasTag("Zombies") and self.Unit:HasTag("Noobs")) or
   (PossibleTargets:HasTag("Noobs") and self.Unit:HasTag("Zombies")) then
  table.insert(Targets, {["Magnitude"] = Nearest, ["FoundTarget"] = PossibleTargets})
end
2 Likes
#3

Going over your points:

  1. NPC’s not switching targets whenever they’re near an enemy.

I noticed there is an issue with the FindNearestTargets logic.

I believe the “Magnitude” here was supposed to be the distance between the NPC and the target, but its setting it to the Nearest variable, which doesn’t get changed at this point in the function, so the NPC thinks every target is math.huge away from themselves. Change Magnitude = Nearest to Magnitude = Distance. Also I think there isn’t any point to the if Distance <= Nearest then check, since again the Nearest variable isn’t getting set at this point, so this is always going to equal to true.

  1. NPC’s getting ‘stuck’ whenever an enemy dies where they’ll walk back and fourth

I believe this issue could arise from this part of your code:

You’re using return in a for loop. This means that if there’s any dead humanoid in the workspace, all your NPCs will think there aren’t any targets existing. Replace return with continue in this case, it will skip the loop to the next iteration instead of ending the execution of the entire function.

  1. The code is poorly formatted.

@BonesIsUseless gives some solid advice about using guard clauses and abstraction to reduce the indentation in the code. As they said, code with lots of indentation (which is what your code happens to be) generally appears less readable.

For example, let’s take the FindUnit loop in your code.

for _, FindUnit in pairs(NoobsFolder:GetChildren()) do
	if FindUnit:IsA("Model") and FindUnit.Name == UnitName then

		-- Actual NPC creation code...

This adds another level of indentation to the code which does the actual creation of the NPC, it can easily be avoided by changing it into this:

local UnitModel = nil

for _, FindUnit in pairs(NoobsFolder:GetChildren()) do
	if FindUnit:IsA("Model") and FindUnit.Name == UnitName then
		UnitModel = FindUnit
		break
	end
end

if UnitModel == nil then
	-- Preferably give an error here to better catch unexpected behavior
	return
end

-- Actual NPC creation code here, replace all occurances of FindUnit with UnitModel...

Next, there’s the if UnitTeam == "..." chains. There appear to be very minimal differences between the creation code for a Noob and for a Zombie, so I believe you can generalize it like this: Create a new “SearchFolder” variable which will be where it searches through for the unit model, based on what team it is. You can do the same thing for the spawns folder and create a “SpawnsFolder” variable for that. It would look something like:

function BaseNPCModule:CreateNPC(UnitName, UnitTeam, Weapon, Special)
	--//Checking to see if the unit exists.
	--//Also checking what team the unit is on as well
	local SearchFolder = if UnitTeam == "Noobs" then NoobsFolder
		elseif UnitTeam == "Zombies" then ZombiesFolder
		elseif UnitTeam == "Neutral" then nil --//TBA
		else nil
	
	if SearchFolder == nil then
		-- Preferably give an error or warning here to better catch unexpected behavior
		return
	end
	
	local UnitModel = nil

	--                       vvvvvvvvvvvv Folder changed!
	for _, FindUnit in pairs(SearchFolder:GetChildren()) do
	-- Rest of the code...


	--//Spawning it to the map.
	local SpawnsFolder = if UnitTeam == "Noobs" then workspace.NoobSpawns
		elseif UnitTeam == "Zombies" then workspace.ZombieSpawns
		elseif UnitTeam == "Neutral" then nil --//TBA
		else nil
	
	if SpawnsFolder == nil then
		-- Preferably give an error or warning here to better catch unexpected behavior
		return
	end

	--                     vvvvvvvvvvvv Folder changed!
	local SelectedSpawns = SpawnsFolder:GetChildren()
	
	-- Rest of the code...

For other parts of the code, @BonesIsUseless already pointed them out.

Other comments:

As per the CollectionService documentation, CollectionService:AddTag returns void (nothing), which means that self.UnitTeam is always going to be nil. Also, Instances have gotten their own AddTag method since, so you can change this part of the code into this:

self.UnitTeam = UnitTeam
self.Unit:AddTag(UnitTeam)

Next:

Not that big of a deal but you’re using game.Workspace and workspace simultaneously, just a bit inconsistent in my opinion.

This:

local Humanoid = PossibleTargets:FindFirstChildWhichIsA("Humanoid")
if Humanoid.Health <= 1 then
	--//They're dead.
	continue
end

Techniiicallyyy, a Humanoid’s health has to be 0 (or less) for them to be dead, so this can accidentally ignore targets which have health equal to 1. Just swap <= 1 with <= 0.

I’m not particularly fond of the way this is structured… Indentation, of course, but also I don’t like the whole use of the while task.wait(0.05) do loop and the additional task.spawn. Of course, I know that self:Pathfind yields, and spawning independent threads makes sure that the NPC doesn’t stutter because of MoveTo, but is doing this really necessary? I would suggest using a RunService connection instead. Also important to note that when the NPC dies and gets destroyed, this loop doesn’t get cleaned up and it results in a memory leak. Here’s the code I’d suggest:

	if not self.Is_Special then
		--//Assuming it's just a basic NPC (E.g. Noob/Zombie Sworder)
		--//Otherwise, the NPC is going to inheritate only the FindNearestTargets and Pathfind functions.
		local timer = 0
		local heartbeatConnection = RunService.Heartbeat:Connect(function(deltaTime)
			timer += deltaTime
			if timer < 0.1 then
				return
			end
			
			timer %= 0.1
			
			local Target = self:FindNearestTargets()

			if Target then
				self:Pathfind(Target)
			end
		end)
		
		local removingConnection; removingConnection = self.Unit.AncestryChanged:Connect(function(child, parent)
			if parent ~= nil or child ~= self.Unit then
				return
			end
			
			removingConnection:Disconnect()
			
			heartbeatConnection:Disconnect()
		end)
	end

Also, perhaps using Pathfinding this often might negatively impact your game’s performance; that’s why I added the timer part, to not make it execute every frame but only after a certain amount of time has passed. I suggest thinking this through a bit more.

I also remembered about abstraction, I think you could abstract the previous code into a separate function like this:

	--//Giving it functionality
	if not self.Is_Special then
		--//Assuming it's just a basic NPC (E.g. Noob/Zombie Sworder)
		--//Otherwise, the NPC is going to inheritate only the FindNearestTargets and Pathfind functions.
		
		defaultTargetFollowBehavior(self)
	end

--[[PRIVATE FUNCS]]
local function defaultTargetFollowBehavior(self)
	local timer = 0
	local heartbeatConnection = RunService.Heartbeat:Connect(function(deltaTime)
		timer += deltaTime
		if timer < 0.1 then
			return
		end

		timer %= 0.1

		local Target = self:FindNearestTargets()

		if Target then
			self:Pathfind(Target)
		end
	end)

	local removingConnection; removingConnection = self.Unit.AncestryChanged:Connect(function(child, parent)
		if parent ~= nil or child ~= self.Unit then
			return
		end

		removingConnection:Disconnect()

		heartbeatConnection:Disconnect()
	end)
end

Other other comments:

Can reformat this as a guard clause:

	if NewPath.Status ~= Enum.PathStatus.Success then
		return
	end

Also, I noticed sometimes your code has guard clauses as one-liners, like this:

if not (guard) then continue end

And sometimes, using 3 lines like this:

if not (guard) then
	return
end

(return/continue change doesn’t matter)
Might consider using one or the other for the entire script, for consistency.

Another thing is, I noticed the code referencing self.Unit.PrimaryPart a lot. Beware that this may sometimes result in errors as the PrimaryPart isn’t necessarily guaranteed to be not nil, so this may lead to unexpected behavior. Consider checking if the PrimaryPart exists first, before using it.

Alright, I’ll stop here. Reply if I missed something.

1 Like
#4

Heya! Sorry for the late response, thought for the CreateNPC function, I’m confused where to put the NPC creation script at. I’d also assuming I’d also need to use local self = setmetatables({}, BaseNPCModule) as well?

1 Like
#5

Yeah! Sorry if it got a little confusing, the -- Rest of the code... parts are where I skipped some code for simplicity’s sake. The NPC creation code (the local self = setmetatable({}, BaseNPCModule) part) should be placed after the if UnitModel == nil then return end check, like this:

function BaseNPCModule:CreateNPC(UnitName, UnitTeam, Weapon, Special)
	--//Checking to see if the unit exists.
	--//Also checking what team the unit is on as well
	local SearchFolder = if UnitTeam == "Noobs" then NoobsFolder
		elseif UnitTeam == "Zombies" then ZombiesFolder
		elseif UnitTeam == "Neutral" then nil --//TBA
		else nil
	
	if SearchFolder == nil then
		return
	end
	
	local UnitModel = nil

	for _, FindUnit in pairs(SearchFolder:GetChildren()) do
		if FindUnit:IsA("Model") and FindUnit.Name == UnitName then
			UnitModel = FindUnit
			break
		end
	end
	
	if UnitModel == nil then
		return
	end
	
	local self = setmetatable({}, BaseNPCModule)

	--//Creating the unit.
	self.Unit = UnitModel:Clone()
	self.Humanoid = self.Unit:FindFirstChildWhichIsA("Humanoid")
	self.Unit.Parent = game.Workspace
	self.UnitTeam = UnitTeam
	self.Is_Special = Special

	self.Unit:AddTag(UnitTeam)

	--//Spawning it to the map.
	local SpawnsFolder = if UnitTeam == "Noobs" then workspace.NoobSpawns
		elseif UnitTeam == "Zombies" then workspace.ZombieSpawns
		elseif UnitTeam == "Neutral" then nil --//TBA
		else nil
	
	if SpawnsFolder == nil then
		return
	end
	
	local SelectedSpawns = SpawnsFolder:GetChildren()
	local Pos = math.random(1,#SelectedSpawns)
	local SpawnPosition = SelectedSpawns[Pos]

	self.Unit.PrimaryPart.CFrame = SpawnPosition.CFrame + Vector3.new(0, 3, 0)
	--//Giving it functionality
	if not self.Is_Special then
		--//Assuming it's just a basic NPC (E.g. Noob/Zombie Sworder)
		--//Otherwise, the NPC is going to inheritate only the FindNearestTargets and Pathfind functions.
		
		defaultTargetFollowBehavior(self)
	end

	return self
end
#6

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.