What do y'all think is best? Any better methods?

Help and Feedback Code Review
#1

hi! do y’all think this is good, or is there a better method to do this? (auto removing from notif stack replicated on all clients for a support system integrated into BAE 2.0)

function pendDestroy(NameToClean)
	local notificationContainer = baseClip:FindFirstChild('Container')
	for index, descendant in next, (baseClip:FindFirstChild('Container'):GetChildren()) do
		if descendant.Name == "Notif Clone" then
			if SupportCalls[NameToClean] ~= nil or {} then
				local Succ,Msg = pcall(function()
				for a,b in pairs(SupportCalls[NameToClean]) do
					if b == descendant then
						for c,d in pairs(Stacks.Notifs) do
							if d == b then
								table.remove(Stacks.Notifs,c)
							end
						end
						b:Destroy()
						table.remove(SupportCalls[NameToClean],a)
						figureNotifs(Stacks.Notifs,notificationContainer)
					   end
				    end
			    end)
		    end
	    end
    end
end
#2
  1. For starters I’d recommend type checking all code. Luau is the future and if you ever work in VSCode with Selene, having the autofill recommendations is a huge time saver: Type checking for beginners!
  2. You’ll find different opinions on this one, but I tend to prefer keeping the indentation level as small as possible. At the deepest nest of your code you’re 8 tabs in. You could easily half this number if instead of using conditionals where “condition == true then” you used “if not condition then continue.” Again, some people have different preferences on this one, but it makes your code flow more vertically instead of horizontally.
  3. The documentation of this code isn’t incredibly clear. There are no comments, but comments aren’t an end-all solution. Don’t add them for no reason. Instead, I’d try to use more descriptive variable names. For example, I have no idea what a “container” is in this context. This goes hand-in-hand with the type checking.
  4. Again, I’m not 100% clear on what you’re trying to accomplish here but I imagine instead of using so many nested for loops you could probably use CollectionService to tag objects for deletion and listen for InstanceAddedSignal to automatically clean them up.
  5. a, b, c, d, etc. are useless as variable names. Takes 1 extra second to name them something more descriptive but makes the code 100% more readable.

My 2 cents~

1 Like
#3

There are definitely some ways to clean up this code.

First, half of your code is all the way on the right side of the screen! Nesting can make code look really gross. To fix this, we can use what are called guard clauses.

I’ve refactored the original here to use guard clauses rather than conditional nesting:

function pendDestroy(NameToClean)
	local notificationContainer = baseClip:FindFirstChild('Container')
	for index, descendant in next, (baseClip:FindFirstChild('Container'):GetChildren()) do
		if descendant.Name ~= "Notif Clone" then continue end -- This is a guard clause!
		if SupportCalls[NameToClean] == nil or {} then continue end -- Also, I've seen you use "nil or {}" after the == sign here. I didn't realize you could do that. Cool!
			
		local Succ,Msg = pcall(function()
			for a,b in pairs(SupportCalls[NameToClean]) do
				if b ~= descendant then continue end

				for c,d in pairs(Stacks.Notifs) do
					if d ~= b then continue end
					table.remove(Stacks.Notifs,c)
				end

				b:Destroy()
				table.remove(SupportCalls[NameToClean],a)
				figureNotifs(Stacks.Notifs,notificationContainer)
			end
		end)
	end
end

Secondly, we can simplify that function you’re pcalling. To me — and correct me if I’m wrong — it looks like you’re trying to remove the descendant from SupportCalls[NameToClean] and Stacks.Notifs. We can use table.find to get the index of the descendant without having to loop through them. I’ve made these changes here:

function pendDestroy(NameToClean)
	local notificationContainer = baseClip:FindFirstChild('Container')
	for index, descendant in next, (baseClip:FindFirstChild('Container'):GetChildren()) do
		if descendant.Name ~= "Notif Clone" then continue end
		if SupportCalls[NameToClean] == nil or {} then continue end
		
		local Succ,Msg = pcall(function()
			-- I've changed quite a bit here. It might not fit in exactly with what you're trying to do, so you might have to change it. The point is that there's a more direct way to remove an object from a table other than iterating through it.
			local position
		
			position = table.find(SupportCalls[NameToClean], descendant)
			if position then
				table.remove(SupportCalls[NameToClean], descendant)
			end

			position = table.find(Stacks.Notifs, descendant)
			if position then
				table.remove(Stacks.Notifs], descendant)
			end

			descendant:Destroy()
			figureNotifs(Stacks.Notifs,notificationContainer)
		end)
	end
end

You’ll then notice some repeated operations for removing from the tables, which we can factor out:

local function removeFromTable(t, value) -- using t here because table is already reserved
	position = table.find(t, descendant)
	if not position then return end
	table.remove(t, position)
end

function pendDestroy(NameToClean)
	local notificationContainer = baseClip:FindFirstChild('Container')
	for index, descendant in next, (baseClip:FindFirstChild('Container'):GetChildren()) do
		if descendant.Name ~= "Notif Clone" then continue end
		if SupportCalls[NameToClean] == nil or {} then continue end
		
		local Succ,Msg = pcall(function()
			removeFromTable(SupportCalls[NameToClean], descendant)
			removeFromTable(Stacks.Notifs, descendant)

			descendant:Destroy()
			figureNotifs(Stacks.Notifs,notificationContainer)
		end)
	end
end

Next, I honestly think we can remove the pcall. The table removing function doesn’t risk erroring. If figureNotifs risks erroring, then we can pcall that individually.

That brings us to our FINAL RESULT:


local function removeFromTable(t, value)
	position = table.find(t, descendant)
	if not position then return end
	table.remove(t, position)
end

function pendDestroy(NameToClean)
	local notificationContainer = baseClip:FindFirstChild('Container')

	for index, descendant in next, (baseClip:FindFirstChild('Container'):GetChildren()) do
		if descendant.Name ~= "Notif Clone" then continue end
		if SupportCalls[NameToClean] == nil or {} then continue end
		
		removeFromTable(SupportCalls[NameToClean], descendant)
		removeFromTable(Stacks.Notifs, descendant)

		descendant:Destroy()
		figureNotifs(Stacks.Notifs,notificationContainer)
	end
end

I may have made some mistakes, so if it doesn’t work, you might have to fiddle around with it. If you know your game well, you should be able to modify it to make it work for you. These are just the changes that I would make.

#4

let me explain the function and provide a video on what it does:

in the basic admin essentials function, i run a check to see if its a support call or not, and if it is, it’s added to a table. note that this is on a client and therefore it runs through several loops. a pcall is due to the fact that server lag times or delays may cause the table to return nil as im using tables to determine who owns which support call (let me know if there are better methods to that lol)

edit: also thanks on the insight about the continue thing, never knew about it until now

and just to clarify this is using Table[Player.UserId] = {} for individual identification

1 Like
#5

also to fix all the errors and stuff, heres the code i decided to go with:

local function returnType(UserId, Notif)
	if Stacks.supportCall[Notif] ~= nil and Stacks.supportCall[Notif] == UserId then
		Stacks.supportCall[Notif] = nil
		return true
	else
		return false
	end
end
function pendDestroy(UserId)
	local notificationContainer = baseClip:WaitForChild('Container')
	for index, descendant in pairs(notificationContainer:GetChildren()) do
			if descendant.Name ~= 'Notif Clone' then continue end
			if returnType(UserId, descendant) ~= true then continue end
		local notifClone = descendant
		for a,b in pairs(Stacks.Notifs) do
			if b == notifClone then
				table.remove(Stacks.Notifs,a)
			end
		end
		notifClone:Destroy()
		figureNotifs(Stacks.Notifs,notificationContainer)
    end
end
1 Like
#6

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.