Is this nicely coded? And is it optimized?

[WizulousThe2nd]

Run down:
This is a SSA (Single Script Architecture), this is the single script on the client, everything else is ran by UI Components, Controllers, and classes. This script runs a few functions whether or not the controller has them, it does work, but I’m questioning whether it’s actually within reason to do this and if this is a WAY to do it, and it doesn’t affect performance. Thoughts?

NOTE: The reason I have to wait on the characterAdded function to fire so I can listen to tools being added is due to it not working immediately if I ran it separately, I’ve tried and debugged this, it needs a bit of loading time and instead of adding a wait in my script, I might as well as wait until the character is added.

NOTE 2: The reason I call all the Classes, Events, etc… Folders is so I can confidently call them in any script without using :WaitForChild as I know they will exist because this script is the first to run and it waits for them.

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")

local Classes = ReplicatedStorage:WaitForChild("Classes")
local Events = ReplicatedStorage:WaitForChild("Events")
local Information = ReplicatedStorage:WaitForChild("Information")
local Libraries = ReplicatedStorage:WaitForChild("Libraries")

local Controllers = script.Parent:WaitForChild("Controllers")

local Player = Players.LocalPlayer
local CollectedControllers = {}	
local Listening = false

local function listening()		
	local function toolAdded(tool)	
		for i, m in CollectedControllers do
			if m.toolAdded then	
				m.toolAdded(tool)
			end
		end	
	end
	
	for _, t in Player.Backpack:GetChildren() do
		if t:IsA("Tool") then	
			toolAdded(t)
		end
	end		
	
	Player.Backpack.ChildAdded:Connect(function(child)	
		if child:IsA("Tool") then
			toolAdded(child)
		end
	end)
end


local function characterAdded(character, humanoid)	
	for i, m in CollectedControllers do
		if m.characterAdded then	
			m.characterAdded(character, humanoid)
		end
	end	
	
	if not Listening then	
		Listening = true	
		listening()
	end
end

local function characterDied(character, humanoid)	
	for i, m in CollectedControllers do
		if m.characterDied then	
			m.characterDied(character, humanoid)
		end
	end	
end

local function characterRemoving(character, humanoid)	
	for i, m in CollectedControllers do
		if m.characterRemoving then	
			m.characterRemoving(character, humanoid)
		end
	end	
end

local function startControllers()	
	table.sort(CollectedControllers, function(a,b)	
		local a = a.LoadDependency or 10	
		local b = b.LoadDependency or 10
		return a < b 
	end)

	for i, m in CollectedControllers do
		if m.start then	
			m.start()
		end
	end	
end

local function loadControllers()		
	for i, v in Controllers:GetChildren() do
		if v:IsA("ModuleScript") then	
			CollectedControllers[v.Name] = require(v)
		end
	end	
	startControllers()	
end

loadControllers()

Player.CharacterAdded:Connect(function(character)
    local Humanoid = character:WaitForChild("Humanoid") 
    characterAdded(character, Humanoid) 

    Humanoid.Died:Connect(function()
        characterDied(character, Humanoid)
    end)
end)

Player.CharacterRemoving:Connect(function(character)
    local Humanoid = character:WaitForChild("Humanoid") 
    characterRemoving(character, Humanoid)
end)

[Iemny]

This thread is more likely meant to be in the topic #help-and-feedback:code-review.

---

But if I were to give my opinion, it is very readable.
For my personal preference, I don’t mind nesting smaller functions like your characterDied and characterRemoving inside of the connection itself. Purely for convenience, that if you need to change something about it that you can find it in the connection itself instead of having to scoll/read somewhere else.

[WizulousThe2nd]

I’ll move it, if I can. Thanks!

[astrovue]

When you’re iterating using loops, you should only define the index variable if you’re going to actually use it. Otherwise, you should leave it blank via _.

for i, v in Controllers:GetChildren() do -- Only use the index variable if you're going to!
    if v:IsA("ModuleScript") then
        CollectedControllers[v.Name] = require(v)
    end
end

-- MODIFIED
for _, v in Controllers:GetChildren() do -- Since you're not using the index variable, use the underscore _
    if v:IsA("ModuleScript") then
        CollectedControllers[v.Name] = require(v)
    end
end

I would also take type annotation into account. When you’re defining parameters, you should use the : operator followed by a class. Only that particular class can be passed in. Any other class will error!

local function characterAdded(character: Model, humanoid: Humanoid) -- STRICTLY models and humanoids can be passed into this function. A different class like a BasePart will error.	
	for _, m in CollectedControllers do
		if m.characterAdded then	
			m.characterAdded(character, humanoid)
		end
	end	
	
	if not Listening then	
		Listening = true	
		listening()
	end
end

You can also do this for variables:

local foo: string = "bar" -- Strictly a string.

Everything else looks solid!