I tried to make it as simple as possible, but i don’t think i can simplify it even more.
the code suffers so much on readability, just look at all those if statements
and i’ve heard that nested if statements are kinda bad
i tried to document my code as much as possible for me and other people to understand without it being too confusing to understand.
basically my code does i made a body guard that aims at random targets, and it looks at a target for a while then switches to a new target.
also its torso and arm also point at the target; the arm doesn’t point at target while the guard is reloading
function Guard:Aim(npcs)
if self.bored == false then -- doesn't look at player while bored
if self.reloading == false and self.shooting == false then --can't pick target while reloading or shooting
if table.find(npcs,self.target) then --if target is in range
if self.targetChange <= time() then--if guard can switch targets
table.remove(npcs,table.find(npcs,self.target)) --removes current target
if #npcs > 0 then --if any targets the guard can switch too
local random = npcs[math.random(0,#npcs)]
if random then --find another random target
self.target = random
self.targetChange = time() + targetChange
end
end
end
else--if target not found
local random = npcs[math.random(0,#npcs)]
if random then --find another random target
self.target = random
self.targetChange = time() + targetChange
end
end
end
if table.find(npcs,self.target) then --only points or looks if target
if self.reloading == false then --doesnt point hand at player while reloading, just looks at targert
TweenService:Create(self.rIK._Shoulder, tweenInfo, {C0 = self.rIK:Solve(self.target.PrimaryPart.Position)}):Play() --move arm
end
TweenService:Create(self.model.PrimaryPart, tweenInfo, {CFrame = CFrame.lookAt(self.model.PrimaryPart.Position, self.target.PrimaryPart.Position)}):Play() --move body
end
end
end
Yeah, that’s quite ugly. Your main if statements read out like:
if not bored… and not reloading and not shooting… and the target exists… and the guard can change targets… then find a new one. Aim at the new one, and if not reloading, aim the arm.
So I tried to clean up while achieving that. I cannot test it of course, but hopefully the comments help explain my interpretation of what’s happening:
function Guard:Aim(npcs)
if (not self.target) or self.bored or (self.reloading and self.shooting) or (self.targetChange > time()) then return end
--[[
Escapes if:
(not self.target) -> no target to look at, or the guard is done
self.bored -> guard is bored
(self.reloading and self.shooting) -> can't change if reloading and shooting
**this may be a logc mistake as self.reloading==false and self.shooting==false is the same as saying
not (self.reloading and self.shooting), is the guard ever doing both?
(self.targetChange > time()) -> target isn't able to change yet
]]
--removing the npc from the table if it exists
if table.find(npcs, self.target) then table.remove(npcs, table.find(npcs, self.target)) end
--clearing target, change time for subsequent calls if there aren't going to be any more targets
self.target = nil
self.targetChange = 0
if #npcs == 0 then return end--if there aren't any more, escape
--picking new target since, at this point, we know there are more & everything is OK to pick another
self.target = npcs[math.random(0, #npcs)]
self.targetChange = time() + targetChange
TweenService:Create(self.model.PrimaryPart, tweenInfo, {CFrame = CFrame.lookAt(self.model.PrimaryPart.Position, self.target.PrimaryPart.Position)}):Play() --move body
if self.reloading then return end--avoids running the next line if reloading
TweenService:Create(self.rIK._Shoulder, tweenInfo, {C0 = self.rIK:Solve(self.target.PrimaryPart.Position)}):Play() --move arm
end
function Guard:ChangeTarget(npcs)
if #npcs > 0 then --if any targets the guard can switch too
local random = npcs[math.random(0,#npcs)]
if random then --find another random target
self.target = random
self.targetChange = time() + targetChange
end
end
end
function Guard:Aim(npcs)
if self.bored == false then -- doesn't look at player while bored
if self.reloading == false and self.shooting == false then --can't pick target while reloading or shooting
if table.find(npcs,self.target) then --if target is in range
if self.targetChange <= time() then--if guard can switch targets
table.remove(npcs,table.find(npcs,self.target)) --removes current target
self:ChangeTarget(npcs)
end
else--if target not found
--Not exactly identical to before, extra check for #npcs > 0
--This feels iffy, might not need this else block
self:ChangeTarget(npcs)
end
end
if table.find(npcs,self.target) then --only points or looks if target
if self.reloading == false then --doesnt point hand at player while reloading, just looks at targert
TweenService:Create(self.rIK._Shoulder, tweenInfo, {C0 = self.rIK:Solve(self.target.PrimaryPart.Position)}):Play() --move arm
end
TweenService:Create(self.model.PrimaryPart, tweenInfo, {CFrame = CFrame.lookAt(self.model.PrimaryPart.Position, self.target.PrimaryPart.Position)}):Play() --move body
end
end
end
Any “outermost” if statement in a function can be simplified by inverting the condition and returning instead:
function Guard:Aim(npcs)
if self.bored == true then -- doesn't look at player while bored
return
end
if self.reloading == false and self.shooting == false then --can't pick target while reloading or shooting
if table.find(npcs,self.target) then --if target is in range
if self.targetChange <= time() then--if guard can switch targets
table.remove(npcs,table.find(npcs,self.target)) --removes current target
self:ChangeTarget(npcs)
end
else--if target not found
--Not exactly identical to before, extra check for #npcs > 0
--This feels iffy, might not need this else block
self:ChangeTarget(npcs)
end
end
if table.find(npcs,self.target) then --only points or looks if target
if self.reloading == false then --doesnt point hand at player while reloading, just looks at targert
TweenService:Create(self.rIK._Shoulder, tweenInfo, {C0 = self.rIK:Solve(self.target.PrimaryPart.Position)}):Play() --move arm
end
TweenService:Create(self.model.PrimaryPart, tweenInfo, {CFrame = CFrame.lookAt(self.model.PrimaryPart.Position, self.target.PrimaryPart.Position)}):Play() --move body
end
end
You now have 3 outside if blocks that can be reorganized independetly. My opinion is the third if block should actually be in its own function, such as AnimateGuard(). I’ve noted that the else block in the second if feels like it doesn’t do anything. You can move the remaining conditions around to have fewer indents if you want, but I think it looks fine at this point.
alright i think i kinda see what you’re thinking, and i probably didn’t give enough details
I used some of the code that you’ve suggested and reduce the repetitiveness
but the thing is, i want the guards to point and look for a target for a set amount of time, and the reason why i have the else for the if statement is so the guard can keep looking at the same target unless the same target is out of range
I also could of made the last chunk of the code the tweening of the arm and body into a different function to reduce confusion, but it somewhat follows the function of the aiming of targets
here’s a vid of the targeting thingy
function Guard:ChangeTarget(npcs)
local random = npcs[math.random(0,#npcs)]
if random then --find another random target
self.target = random
self.targetChange = time() + targetChange
print('change target')
end
end
function Guard:Aim(npcs)
if self.bored == true then return end --doesn't look at player while bored
if self.reloading == false and self.shooting == false then --can't pick target while reloading or shooting
if table.find(npcs,self.target) then --if target is in range
if self.targetChange <= time() then --if guard can switch targets
table.remove(npcs,table.find(npcs,self.target)) --removes current target so to see if the guard can find another target to switch to
if #npcs > 0 then --if any targets the guard can switch too
self:ChangeTarget(npcs)
end
end
else --if target not found
self:ChangeTarget(npcs)
end
end
if table.find(npcs,self.target) then --only points or looks if theres a target
if self.reloading == false then --doesnt point hand at player while reloading, just looks at targert
TweenService:Create(self.rIK._Shoulder, tweenInfo, {C0 = self.rIK:Solve(self.target.PrimaryPart.Position)}):Play() --move arm
end
TweenService:Create(self.model.PrimaryPart, tweenInfo, {CFrame = CFrame.lookAt(self.model.PrimaryPart.Position, self.target.PrimaryPart.Position)}):Play() --move body
end
end