Help on suggestions for a cleaner code

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)
1 Like

doesnt look messy to me. this is how my scripts normally look

1 Like

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

Usually, this means something can be more organized! Of course, this is normal with a lot of logic that cannot be simplified.

You can just use if success and bool then.

If checkRay is true, then automatically, it’s not nil.
All you need is if checkRay then.

This can be simplified using:

checkRay, position, distance = castRay(currentTarget or v)

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.

Hope this helps.

Just fixed it a bit. Good?

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).

I have a problem in my code.

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.