This feels a little hacky. Is there a better way of writing this script?

Help and Feedback Code Review
1

I created script for a tower defense game, that uses a detector. With ZonePlus, I find out what zombies are inside of the detector, and then with Vector3 Magnitude, I find out how far away the zombie is, and store all of the info in a table. Then I sort the table, and get the first variable.

When this script was in development, it had the chance to break when a zombie was killed. Is there a better way of writing the script shown below? I also feel that this is a little hacky way of writing this.

Once the final script passes Code Review, I will add type checking to it.

local ReplicatedStorage: ReplicatedStorage = game:GetService("ReplicatedStorage")
local TweenService: TweenService = game:GetService("TweenService")

local BulletInfo = TweenInfo.new(3, Enum.EasingStyle.Linear, Enum.EasingDirection.InOut, 0, false)

local Zone = require(ReplicatedStorage.Zone)

local TargetList = {}

local TroopDetection = Zone.new(script.Parent.Detector.Detector)

TroopDetection.partEntered:Connect(function(Item)
	TargetList[Item.Parent] = true
end)
TroopDetection.partExited:Connect(function(Item)
	if TargetList[Item.Parent] then
		TargetList[Item.Parent] = nil
	end
end)

while true do
	task.wait(1)
	local MagnitudeList = {}
	for Target, Value in pairs(TargetList) do
		if Target then
			if Target:IsA("Model") then
				if Target.PrimaryPart then
					if Target.PrimaryPart.Position then
						table.insert(MagnitudeList, {
							["Magnitude"] = (Target.PrimaryPart.Position - script.Parent.Actor.PrimaryPart.Position).Magnitude,
							["Target"] = Target
						})
						print((Target.PrimaryPart.Position - script.Parent.Actor.PrimaryPart.Position).Magnitude)
					end
				end
			end
		end
		table.sort(MagnitudeList, function(MagnitudeLow, MagnitudeHigh)
			return MagnitudeLow.Magnitude < MagnitudeHigh.Magnitude
		end)
		print(MagnitudeList[1])
		local ChosenTargetData = MagnitudeList[1]
		if ChosenTargetData then
			local Ammo = script.Parent.Ammo:Clone()
			Ammo.Position = script.Parent.LineOfDirection.Dispenser.Position
			Ammo.Parent = game:GetService("Workspace")
			local Tween: Tween = TweenService:Create(Ammo, BulletInfo, {Position = script.Parent.LineOfDirection.ToPoint.Position})
			Tween:Play()
			local AmmoConnection
			AmmoConnection = Ammo.Touched:Connect(function(Hit)
				if Hit.Parent == ChosenTargetData.Target then
					Ammo:Destroy()
					ChosenTargetData.Target.Humanoid.Health -= 1
					AmmoConnection:Disconnect()
				end
			end)
		end
	end
end

Thanks in advance!

1 Like
2

Change the local variables that define the services you use in the script to this:

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local TweenService = game:GetService("TweenService")
3

Here try this:

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local TweenService = game:GetService("TweenService")
local BulletInfo = TweenInfo.new(3, Enum.EasingStyle.Linear, Enum.EasingDirection.InOut, 0, false)
local Zone = require(ReplicatedStorage.Zone)
local TroopDetection = Zone.new(script.Parent.Detector.Detector)
local ScriptParent = script.Parent
local Dispenser = ScriptParent.LineOfDirection.Dispenser
local ActorPart = ScriptParent.Actor.PrimaryPart
local TargetList = {}

TroopDetection.partEntered:Connect(function(Item)
	TargetList[Item.Parent] = true
end)

TroopDetection.partExited:Connect(function(Item)
	if TargetList[Item.Parent] then
		TargetList[Item.Parent] = nil
	end
end)

while true do
	task.wait(1)
	local MagnitudeList = {}
	
	for Target, Value in pairs(TargetList) do
		if Target and Target:IsA("Model") then
			if Target.PrimaryPart and Target.PrimaryPart.Position then
				table.insert(MagnitudeList, {
					["Magnitude"] = (Target.PrimaryPart.Position - ActorPart.Position).Magnitude,
					["Target"] = Target
				})
				
				print((Target.PrimaryPart.Position - ActorPart.Position).Magnitude)
			end
		end
		
		table.sort(MagnitudeList, function(MagnitudeLow, MagnitudeHigh)
			return MagnitudeLow.Magnitude < MagnitudeHigh.Magnitude
		end)
		print(MagnitudeList[1])
		
		local ChosenTargetData = MagnitudeList[1]
		if ChosenTargetData then
			local Ammo = ScriptParent.Ammo:Clone()
			Ammo.Position = Dispenser.Position
			Ammo.Parent = workspace
			
			local Tween = TweenService:Create(Ammo, BulletInfo, {Position = script.Parent.LineOfDirection.ToPoint.Position})
			Tween:Play()
			
			local AmmoConnection
			AmmoConnection = Ammo.Touched:Connect(function(Hit)
				if Hit.Parent == ChosenTargetData.Target then
					Ammo:Destroy()
					ChosenTargetData.Target.Humanoid.Health -= 1
					AmmoConnection:Disconnect()
				end
			end)
		end
	end
end

I just added more variables to shorten the code and make it easier to read, I also compiled some if statements that don’t really need to be seperate.