Can anyone help answer my questions about my code

Help and Feedback Code Review
#1

i feel im not satisfied with this section of the code

first of all theres way too many nested if statements
and next this code isn’t event based driven so it heavily relies on runservice to do checks constantly

local function GuardUpdate()
	for _, guard in pairs(Guards) do
		local characters, moving = FindCharacters(PlayersAlive, guard.model, guardRange, CharacterShot)
		
		if #characters == 0 then -- if no character nearby
			guard.target = nil
			guard:Reload()
			guard:Bored()
		else --if characters near guard
			if RedLightGreenLight.RedLight then -- if red light
				if guard.ammo_count > 0 then -- if ammo above 0
					if #moving > 0 then -- if any moving players
						local random = moving[math.random(#moving)]
						if random then -- finds a random moving character
							guard:Shoot(random)
						end
					else -- if no one is moving
						if guard.reload_delay <= time() then -- waits a while before reloading
							guard:Reload()
						end
						guard:Bored()
					end
				else -- no ammo left
					guard:Reload()
				end
			else -- if green light
				guard:Reload()
				guard:Bored()
			end
			if guard.bored == false then -- aims at players
				guard:PickTarget(characters)
				guard:PointTarget(characters)
			end
		end
	end
end
1 Like
#2

To avoid nesting:

Instead of

if (condition) then
    -- Code
end

Do

if (not condition) then return end

For NPCs, you can use a while loop with a task.wait(0.2) and it should still be smooth.
You are looping through each NPC. I recommend that you wrap each NPC in a coroutine so that it if an NPC yields for some reason the loop can still continue on.

for _, guard in pairs(Guards) do
    coroutine.resume(coroutine.create(function()
        -- Your NPC code
    end))
end
2 Likes
#3

i would agree with you on the first part, but it’s backwards logic which would complicate things even more if looking back at the code

wouldn’t while loop halts the entire code

while true do
    for _, guard in pairs(Guards) do
    --rest of the code
    end
    task.wait(0.2)
end

print("won't print")
1 Like
#4

You should make the condition return early to prevent more nesting instead of putting them in the else block:

if (condition) then
     doSmth()
     return --return early
end

-- else if not condition
doSmthElse()



Example

		if #characters == 0 then -- if no character nearby
			guard.target = nil
			guard:Reload()
			guard:Bored()
			return
		end

		if RedLightGreenLight.RedLight then -- if red light

I think all of this if nesting can be solved by only using guard clauses.

Guard Clauses:

3 Likes
#5

You can use a different keyword if you are working inside a loop. continue can be used to halt current cycle and tell the loop to continue from the next one. If on the other hand you want the loop to stop but not to return you can use break instead

1 Like
#6

i really tried, but the nested if statements i can’t stop it all
and doing this, i don’t even know if the code has the same logic or if its gonna work at all

and does this drastically improve readability or does make it worse to comprehend?

local function GuardUpdate()
	for _, guard in pairs(Guards) do
		local characters, moving = FindCharacters(PlayersAlive, guard.model, guardRange, CharacterShot)

		if #characters == 0 then-- if no character nearby
			guard.target = nil
			guard:Reload()
			guard:Bored()
			return
		end
		
		if guard.bored == false then -- aims at players
			guard:PickTarget(characters)
			guard:PointTarget(characters)
		end

		if not RedLightGreenLight.RedLight or guard.ammo_count <= 0 then --if green light or ammo is 0
			guard:Reload()
			guard:Bored()
			return
		end
		
		if #moving <= 0 then-- if any moving players
			if guard.reload_delay <= time() then -- waits a while before reloading
				guard:Reload()
			end
			guard:Bored()
			return
		end

		local random = moving[math.random(#moving)]
		if random then -- finds a random moving character
			guard:Shoot(random)
		end
		
	end
end
#7

im talking about the while loop not being able to continue to the rest of the code underneath the while loop because it repeats indefinitely unless you break/return it

1 Like
#8

Personally, I find this easier to read, as i think the scopes are clearer. But my favourite thing to improve readability is to use comments preceding each block to explain what/why it’s there. (I find if they are on the end of a line it’s awkward separating them from the code visually and evenly)
For example:

--Does the guard have ammo?
if guard.ammo <= 0 then
    guard:reload()
end
2 Likes