How can I make this code NOT painful to look at?

Help and Feedback Code Review
#1

I have this pathfinding + damage script, it’s kind of long (230 lines), and it’s EXTREMELY unreadable, even for a novice like me :sob:

I figured I’d ask on here since My knowledge about clean code is next to zero.

I’ll take literally any advice on how to make this not an eyesore

local LOSsize = Vector3.new(75,25,75)
local Dummy = script.Parent
local PF = game:GetService("PathfindingService")
local DummyHumanoid = Dummy.HumanoidRootPart
local RS = game:GetService("RunService")
local players = game:GetService("Players")
local db = false
local seed = 0
local isJumping = false
local Humanoid = Dummy:WaitForChild("Humanoid")
local forwardOffset = 2
local Size = Vector3.new(3,7,4)
local db = false

function AttackArea()
	local Area = Instance.new("Part")
	Area.Name = "AttackBox"
	Area.Parent = DummyHumanoid
	Area.CFrame = DummyHumanoid.CFrame * CFrame.new(0,0, -forwardOffset)
	Area.Size = Size

	Area.Transparency = 0.5
	Area.CanCollide = false
	Area.Anchored = true
	game:GetService("Debris"):AddItem(Area, 5)
end

function createAreapreview()
	local part = Instance.new("Part")
	part.Name = "LoSChecker"
	part.Parent = Dummy.HumanoidRootPart
	part.CFrame = Dummy.HumanoidRootPart.CFrame
	part.Size = LOSsize
	part.Color = Color3.fromRGB(255, 0, 4)

	part.Anchored = true
	part.Transparency = 0.6
	part.CanCollide = false
end
createAreapreview()

local DMGsize = Vector3.new(10,7,10)

function createDamageArea()
	local part = Instance.new("Part")
	part.Name = "DamageDealer"
	part.Parent = Dummy.HumanoidRootPart
	part.CFrame = Dummy.HumanoidRootPart.CFrame
	part.Size = DMGsize
	part.Color = Color3.fromRGB(230, 255, 120)

	part.Anchored = true
	part.Transparency = 0.6
	part.CanCollide = false
end
createDamageArea()

local parameterTable = {
	AgentRadius = 3,
	AgentHeight = 2,
	AgentCanJump = true,
	AgentCanClimb = false,
	WaypointSpacing = 4
}

local dmgDealer = DummyHumanoid.DamageDealer
local losChecker = DummyHumanoid.LoSChecker
local path = PF:CreatePath(parameterTable)

local isJumping = false
local seed = 0

function TraversePath(closestPlayer)
	seed += 1
	local currentSeed = seed

	local waypoints = path:GetWaypoints()
	if path.Status == Enum.PathStatus.Success and #waypoints > 0 then
		for i = 2, #waypoints, 1 do
			local waypoint = waypoints[i]

			if waypoint.Action == Enum.PathWaypointAction.Jump then
				isJumping = true
				Dummy.Humanoid.Jump = true
			end

			Dummy.Humanoid:MoveTo(waypoints[i].Position)
			Dummy.Humanoid.MoveToFinished:Wait()

			if waypoint.Action ~= Enum.PathWaypointAction.Jump then
				isJumping = false
			end

			if not isJumping and seed ~= currentSeed then break end
		end
	end
end

local Weapon = Dummy:FindFirstChildWhichIsA("Tool")
local ComboAnim = Weapon:FindFirstChild("ComboAnim")
local HeavySwing = Weapon:FindFirstChild("HeavySwing")
local animator = Humanoid:WaitForChild("Animator")
local combo = animator:LoadAnimation(ComboAnim)
local heavyattack = animator:LoadAnimation(HeavySwing)
local attackSeed = 0
local connection = nil
local Stab = Weapon:WaitForChild("Stab")
local event = game.ReplicatedStorage:WaitForChild("MeleeEvents"):WaitForChild("DaggerEvents").SoundEvent

local AttackChances = {
	[heavyattack] = 60,
	[combo] = 40
}



function Attack()
	local totalChance = 0 
	for _,chance in pairs(AttackChances) do
		totalChance += chance
	end

	local rng = math.random(1, totalChance)

	for option, chance in pairs(AttackChances) do
		rng -= chance
		if rng <= 0 then
			if option == heavyattack then
				heavyattack:Play()
			elseif option == combo then
				combo:Play()
			end
		end
	end
end

local stabId = "rbxassetid://16815968728"

heavyattack.KeyframeReached:Connect(function(KeyframeName)
	local FrontCFrame = DummyHumanoid.CFrame * CFrame.new(0,0, -forwardOffset)
	if KeyframeName == "AttackFrame" then
		AttackArea()
		local Punish = workspace:GetPartBoundsInBox(FrontCFrame, Size)
		for _,v in Punish do
			if v.Parent:FindFirstChild("Humanoid") and players:GetPlayerFromCharacter(v.Parent) then
				if db == false then
					db = true	
					v.Parent:FindFirstChild("Humanoid"):TakeDamage(40)
					print("Heavy")
					event:FireAllClients(stabId, Weapon)
					heavyattack.Ended:Wait()
					db = false
					break
				end
			end
		end
	end
end)

combo.KeyframeReached:Connect(function(KeyframeName)
	local FrontCFrame = DummyHumanoid.CFrame * CFrame.new(0,0, -forwardOffset)
	if string.find(KeyframeName, "Combo") then
		AttackArea()
		local Punish = workspace:GetPartBoundsInBox(FrontCFrame, Size)
		for _,v in Punish do
			if v.Parent:FindFirstChild("Humanoid") and players:GetPlayerFromCharacter(v.Parent) then
				v.Parent:FindFirstChild("Humanoid"):TakeDamage(13)
				print("Light")
				combo.Ended:Wait()
				db = false
				break
			end
		end
	end
end)

local attacking = false

RS.Heartbeat:Connect(function()
	losChecker.CFrame = Dummy.HumanoidRootPart.CFrame
	dmgDealer.CFrame = Dummy.HumanoidRootPart.CFrame
	local LOS = workspace:GetPartBoundsInBox(losChecker.CFrame, losChecker.Size)
	local closestPlayer = nil

	for _, v in pairs(LOS) do
		if v.Parent:FindFirstChild("Humanoid") then
			local targetPlayer = game.Players:GetPlayerFromCharacter(v.Parent)

			if targetPlayer then
				if closestPlayer == nil or (Dummy.HumanoidRootPart.Position - targetPlayer.Character.HumanoidRootPart.Position).Magnitude < (Dummy.HumanoidRootPart.Position - closestPlayer.Character.HumanoidRootPart.Position).Magnitude then
					closestPlayer = targetPlayer
				end
			end
		end
	end

	if closestPlayer then
		losChecker.Color = Color3.fromRGB(123, 255, 93)

		if not isJumping then
			task.spawn(TraversePath, closestPlayer)
		end

		local success, errorMessage = pcall(function()
			path:ComputeAsync(Dummy.HumanoidRootPart.Position, closestPlayer.Character.HumanoidRootPart.Position)
		end)
	else
		losChecker.Color = Color3.fromRGB(255, 0, 4)
		Dummy.Humanoid:Move(Vector3.new(0, 0, 0))
	end


	----------------DAMAGE DEALER---------------
	local DMG = workspace:GetPartBoundsInBox(dmgDealer.CFrame, dmgDealer.Size)
	for _,v in DMG do
		if v.Parent:FindFirstChild("Humanoid") then
			local plr = players:GetPlayerFromCharacter(v.Parent)
			if plr then
				dmgDealer.Color = Color3.fromRGB(108, 135, 255)
				if attacking == true then return end
				if attacking == false then
					Attack()
					attacking = true
					coroutine.wrap(function()
						task.wait(4)
						attacking = false
					end)()
				end
			else
				dmgDealer.Color = Color3.fromRGB(230, 255, 120)
			end
		end
	end
end)
3 Likes
#2

Try to organize the code with Roblox services at the top, followed by variables in the order from primitive to more constructed. Sometimes, you need to initialize things ahead of time and a section for initializing which is often only executions. Follow it by local functions/functions and then finally binding and executions.

#3

Hi, it’s a bit hard to go over evertything on the forum but for instance something that could help reduce the clutter is to create a module script along the lines of “AreaModule” so you would move there

  • AttackArea
  • createAreapreview
  • createDamageArea

You would probably double check what parameter you can move, and which ones you only need to pass i.e you could call…

AreaModule.AttackArea(DummyHumanoid, forwardOffset, Size)

or move the initialization of forwardOffset and Size directly to the module (you need to figure that out based on if you use it somewhere else in the code or just there)
Also you could replace big statements like…

(Dummy.HumanoidRootPart.Position - targetPlayer.Character.HumanoidRootPart.Position).Magnitude < (Dummy.HumanoidRootPart.Position - closestPlayer.Character.HumanoidRootPart.Position).Magnitude

with two locals that describe what you are doing making the code more readable like so:

local distanceToTargetPlayer = (Dummy.HumanoidRootPart.Position - targetPlayer.Character.HumanoidRootPart.Position).Magnitude
local distanceToClosestPlayer = (Dummy.HumanoidRootPart.Position - closestPlayer.Character.HumanoidRootPart.Position).Magnitude

if distanceToTargetPlayer > distanceToClosestPlayer then ...

Another good trick to avoid long nesting into ifs is to revert the condition into the exit one, like instead of

 if targetPlayer then

and nesting everything, you do

if not targetPlayer then continue end

and write the code past that, that way you prevent nesting and still are safe that past that line you can count on the targetPlayer being available.

Hope this tips helped, best way to learn is practice, practice, practice. Keep it up!

#4

Gotta love setting db = false twice at the top

Also instead of coroutine.wrap()() you can use task.spawn() which automatically calls the function so you don’t need () at the end. You actually do use both for some reason.

Vector3.zeros is the same as .new(0,0,0)

Sometimes you use Dummy.HumanoidRootPart when you already have the DummyHumanoid variable.

Sometimes you use Dummy.Humanoid despite having the Humanoid variable.

#5

You need to stick to a consistent coding style. For instance, camelCase for variable names and PascalCase for function names. Right now, you’re mixing a lot of these together, which makes it feel messy.

You could also organize it a bit. Typically, I put all services at the top, then constants below them, then other variables below that, followed by functions last.

You might also be able to use ModuleScripts to split some of the code up into smaller chunks.

This is an older guide, but still pretty relevant: Roblox Lua Style guide

3 Likes
#6

Just categorize the variables

For example:

-- Services
local RunService = game:GetService("RunService")

-- Variables
local Seed = 0

Then it wont be confusing
Also you defined “db” false twice in the first openning lines

Also rather then defining variables as you continue into the code, just keep it in the openning lines

#7

pretty sure coroutines are faster than spawn(), but not sure about task.spawn (if they are even different)

#8

coroutine methods and task.spawn can both create a thread

task.spawn returns a thread which can be used in coroutine methods because too, because it’s still the same type of thread

#9

I forgot to delete that line when I was trying to reorganise something, and didn’t notice it until yesterday

#10

I’ve never used module scripts, I’ll look into them and see what I can do