I have this code that looks very messy and I want to know what I should do to clean this.
local RunService = game:GetService("RunService")
local replicatedStorage = game:GetService("ReplicatedStorage")
local Remotes = replicatedStorage:WaitForChild("Remotes")
local Modules = replicatedStorage:WaitForChild("Modules")
local ToolData = require(script.Parent:WaitForChild("ToolData"))
local character = script.Parent.Parent
local humanoid = character:WaitForChild("Hero")
local animator = humanoid:FindFirstChildOfClass("Animator")
local RemotesFolder = character:WaitForChild("RemotesFolder")
local npcs = workspace:WaitForChild("Npcs")
local animationTable = {
[0] = script:WaitForChild("Attack1"),
}
local tool = character:WaitForChild("Bow")
local hitbox = tool:WaitForChild("Hitbox")
local firePoint = hitbox:WaitForChild("FirePoint")
local range = ToolData.MaxRange
local animationCombo = 0
local currentTime = tick()
local canShoot = true
local damageCooldown = 1.5
local currentTarget = nil
local canAttack = character:WaitForChild("CanAttack")
local track = nil
local function playAnimation()
if animationTable[animationCombo] then
local anim = animationTable[animationCombo]
track = animator:LoadAnimation(anim)
track:Play()
track.Stopped:Wait()
canShoot = true
end
animationCombo += 1
if animationCombo > #animationTable then
animationCombo = 0
end
end
local function castRay(target)
if target.PrimaryPart ~= nil then
local raycastParams = RaycastParams.new()
raycastParams.FilterDescendantsInstances = {npcs}
raycastParams.FilterType = Enum.RaycastFilterType.Whitelist
raycastParams.IgnoreWater = true
local rayOrigin = character.PrimaryPart.Position
local rayDestination = target.PrimaryPart.Position
local rayDirection = (rayDestination - rayOrigin)
local raycastResult = workspace:Raycast(rayOrigin, rayDirection, raycastParams)
if raycastResult then
local distance = math.floor(raycastResult.Distance)
if distance <= range then
currentTarget = target
return true, raycastResult.Position, distance
else
currentTarget = nil
return false, nil, nil
end
else
currentTarget = nil
return false, nil, nil
end
else
currentTarget = nil
return false, nil, nil
end
end
local function PredictNextPosition(targetPosition, targetVelocity)
if targetPosition ~= nil and targetVelocity ~= nil then
local t = ToolData.ProjectileTime
local endPosition = targetPosition + targetVelocity * t
return endPosition
end
end
local function getPlayer()
local player = Remotes.GetPlayerBindable:Invoke()
if player then
return player
end
end
local player = getPlayer()
local function findTarget()
local checkRay, position, distance = nil, nil, nil
for i,v in ipairs(npcs:GetChildren()) do
if currentTarget == nil then
checkRay, position, distance = castRay(v)
else
checkRay, position, distance = castRay(currentTarget)
end
if currentTarget ~= nil then
local savedTarget = currentTarget
local targetRoot = currentTarget.PrimaryPart
local human = currentTarget:FindFirstChildWhichIsA("Humanoid")
local dist = (character.HumanoidRootPart.Position - targetRoot.Position).Magnitude
if human and human.Health <= 0 then
currentTarget = nil
end
if distance ~= nil and distance > range then
currentTarget = nil
end
if player and human and human.Health > 0 and currentTarget ~= nil and targetRoot ~= nil then
local success, bool = pcall(function()
return Remotes.CheckOnScreen:InvokeClient(player, targetRoot, {npcs})
end)
if success and bool == true then
local characterRoot = character.PrimaryPart
character:PivotTo(CFrame.lookAt(characterRoot.Position, position * Vector3.new(1, 0, 1) + position * Vector3.new(0, 1, 0)))
if checkRay ~= nil and checkRay == true then
if tick() - currentTime >= damageCooldown then
currentTime = tick()
playAnimation()
checkRay, position, distance = castRay(savedTarget)
local nextPosition = PredictNextPosition(position, targetRoot.AssemblyLinearVelocity)
if checkRay ~= nil and checkRay == true and nextPosition ~= nil and canShoot == true and human and human.Health > 0 then
RemotesFolder.FireProjectile:Fire(firePoint, nextPosition)
canShoot = false
end
end
end
end
end
end
end
end
RunService.Heartbeat:Connect(function()
if character.PrimaryPart ~= nil and canAttack.Value == true then
findTarget()
end
end)
For this you could make a variable or table for what to return and then remove all the returns and just have one at the end returning the variable/table
You could add notes on what aspect of the script does before each function. Therefore, if you come back in the future to edit it, you know what function does what.
Alternatively, you could also add a note where variables are (If you have any) so you can find the easier later on.
local RunService = game:GetService("RunService")
local replicatedStorage = game:GetService("ReplicatedStorage")
local Remotes = replicatedStorage:WaitForChild("Remotes")
local Modules = replicatedStorage:WaitForChild("Modules")
local ToolData = require(script.Parent:WaitForChild("ToolData"))
local character = script.Parent.Parent
local humanoid = character:WaitForChild("Hero")
local animator = humanoid:FindFirstChildOfClass("Animator")
local RemotesFolder = character:WaitForChild("RemotesFolder")
local npcs = workspace:WaitForChild("Npcs")
local animationTable = {
[0] = script:WaitForChild("Attack1"),
}
local tool = character:WaitForChild("Bow")
local hitbox = tool:WaitForChild("Hitbox")
local firePoint = hitbox:WaitForChild("FirePoint")
local range = ToolData.MaxRange
local animationCombo = 0
local currentTime = tick()
local canShoot = true
local damageCooldown = 1.5
local currentTarget = nil
local canAttack = character:WaitForChild("CanAttack")
local track = nil
local function playAnimation()
if animationTable[animationCombo] then
local anim = animationTable[animationCombo]
track = animator:LoadAnimation(anim)
track:Play()
track.Stopped:Wait()
canShoot = true
end
animationCombo += 1
if animationCombo > #animationTable then
animationCombo = 0
end
end
local function castRay(target)
if target.PrimaryPart ~= nil then
local raycastParams = RaycastParams.new()
raycastParams.FilterDescendantsInstances = {npcs}
raycastParams.FilterType = Enum.RaycastFilterType.Whitelist
raycastParams.IgnoreWater = true
local rayOrigin = character.PrimaryPart.Position
local rayDestination = target.PrimaryPart.Position
local rayDirection = (rayDestination - rayOrigin)
local raycastResult = workspace:Raycast(rayOrigin, rayDirection, raycastParams)
if raycastResult then
local distance = math.floor(raycastResult.Distance)
if distance <= range then
currentTarget = target
return true, raycastResult.Position, distance
end
end
end
currentTarget = nil
return false, nil, nil
end
local function PredictNextPosition(targetPosition, targetVelocity)
if targetPosition ~= nil and targetVelocity ~= nil then
local t = ToolData.ProjectileTime
local endPosition = targetPosition + targetVelocity * t
return endPosition
end
end
local function getPlayer()
local player = Remotes.GetPlayerBindable:Invoke()
if player then
return player
end
end
local player = getPlayer()
local function reset()
if track ~= nil then
track:Stop()
end
currentTarget = nil
canShoot = true
end
local function findTarget()
local checkRay, position, distance = nil, nil, nil
for i,v in ipairs(npcs:GetChildren()) do
checkRay, position, distance = castRay(currentTarget or v)
if currentTarget ~= nil then
local savedTarget = currentTarget
local targetRoot = currentTarget.PrimaryPart
local human = currentTarget:FindFirstChildWhichIsA("Humanoid")
if human and human.Health > 0 then
if human and human.Health <= 0 then
currentTarget = nil
end
if distance > range then
currentTarget = nil
end
if player and human and human.Health > 0 and currentTarget ~= nil and targetRoot ~= nil and checkRay == true then
local success, bool = pcall(function()
return Remotes.CheckOnScreen:InvokeClient(player, targetRoot, {npcs})
end)
if success and bool then
local characterRoot = character.PrimaryPart
character:PivotTo(CFrame.lookAt(characterRoot.Position, position * Vector3.new(1, 0, 1) + position * Vector3.new(0, 1, 0)))
if tick() - currentTime >= damageCooldown then
currentTime = tick()
playAnimation()
checkRay, position, distance = castRay(savedTarget)
local nextPosition = PredictNextPosition(position, targetRoot.AssemblyLinearVelocity)
if checkRay ~= nil and checkRay == true and nextPosition ~= nil and canShoot == true and human and human.Health > 0 then
RemotesFolder.FireProjectile:Fire(firePoint, nextPosition)
canShoot = false
else
reset()
end
end
end
end
else
reset()
end
end
end
end
RunService.Heartbeat:Connect(function()
if character.PrimaryPart ~= nil and canAttack.Value == true then
findTarget()
end
end)
Lots of minor issues. Use selene to highlight problems in your code like unused variables. Don’t use variable names like i or v. Don’t have 8 tabs of indentation.
The major issues making your code messy are: using global variables and if statements everywhere to manage state, and having huge functions with way too many responsibilities (and very bad names, they don’t actually describe what they do).
local success, bool = pcall(function()
return Remotes.CheckOnScreen:InvokeClient(player, targetRoot, {npcs})
end)
This part detects if the player camera can see the target. The problem is that it fires so many times that it is warning me “Player appears to be spamming remote events”.
But it looks like you are spamming remotes, this could possibly fire every 4ms (i.e. heartbeat). You may need a flopFlop variable (with a cooldown) to prevent it continually badgering the server.
this part can be very simplified as no matter what happens
it is either true, raycastResult.Position,distance
or false,nil,nil and you do not need to return nil so it is just false
so replace this with
if raycastResult then
local distance = math.floor(raycastResult.Distance)
if distance <= range then
currentTarget = target
return true, raycastResult.Position, distance
end
end
end
currentTarget = nil
return false
and this
there is no need for the local variables behind the for loop because they are never used outside of the loop, and you can simplify the if and elses by
local checkRay, position, distance=castRay(if currentTarget then currentTarget else v)
or
local checkRay, position, distance =castRay(currentTarget or v)
I am also not a fan of how nested this is, but I bet you can find a way to clean it.