Seeking constructive criticism on how this script can be further improved . Basically, everytime a listener is fired, the LoadEvent function iterates through the Modules table and calls a function depending on the event name.
local Players = game:GetService('Players')
local Modules = {}
for _, Module in ipairs(script:GetChildren()) do
local Required = require(Module)
table.insert(Modules, Required)
end
local function LoadEvent(Event) --> Function
return function(Player) --> Void
for _, Module in ipairs(Modules) do
local Hook = Module[Event]
if Hook then
Hook(Player)
end
end
return nil
end
end
local Added = LoadEvent('Added')
for _, Player in ipairs(Players:GetPlayers()) do
Added(Player)
end
Players.PlayerAdded:Connect(Added)
Players.PlayerRemoving:Connect(LoadEvent('Removed'))
Your ServerInit looks nice. At this point, in my opinion, there isn’t, perhaps yet, anything much to improve. I see you require your modules at the top of the script, which is alright, but you can require directly instead of allocating a new Required table. You can save some memory and load them directly into your Modules table.
-- I avoided table.insert() for the sake of readability.
Modules[#Modules+1] = require(module)
Instead of allocating Hook variable (function), you could check the type of Module[event] directly. Saves some memory as well.
for _, module in ipairs(Modules) do
if type(module[event]) ~= "function" then continue; end
module[event](player)
end
Lastly, you don’t have to loop through players list, because unless there is an error in the script, it will certainly start running before anyone joins.
I’m a little short on time right now, so I’ll update the post later. If the following script doesn’t work, you can most likely still continue following that logic.
local Players = game:GetService('Players')
local Modules = {}
for _, module in ipairs(script:GetChildren()) do
Modules[#Modules+1] = require(module)
end
local function LoadEvent(event)
return function(player)
for _, module in ipairs(Modules) do
if type(module[event]) ~= 'function' then continue; end
module[event](player)
end
return nil
end
end
Players.PlayerAdded:Connect(LoadEvent('Added'))
Players.PlayerRemoving:Connect(LoadEvent('Removed'))
Essence has made good points regarding improvement, but you should consider caching the required functions instead of looping through the modules again for each player, you already do some sort of caching but that isn’t much, your code should be flexible. For debugging purposes, always emit some sort of warns or debug info, that’s important.
local Players = game:GetService('Players')
local RequiredModules = {}
local CachedLoadedEvents = {}
for _, module in ipairs(script:GetChildren()) do
module = require(module)
table.insert(Modules, module)
end
local function LoadEvent(event)
return function (player)
local cachedLoadedEvent = CachedLoadedEvents[Event]
if cachedLoadedEvent then
cachedLoadedEvent(player)
else
for _, module in ipairs(RequiredModules) do
local callBack = module[event]
if callBack then
CachedLoadedEvents[Event] = callBack
callBack(Player)
else
warn(("Event %s not found in module: %s"):format(event, module:GetFullName()))
end
end
end
end
end
for _, player in ipairs(Players:GetPlayers()) do
LoadEvent('Added')(player)
end
Players.PlayerAdded:Connect(LoadEvent('Added'))
Players.PlayerRemoving:Connect(LoadEvent('Removed'))
In some cases returning nil is important, consider the code:
local function a()
end
print(type(a())) -- error, no argument
local function b()
return nil
end
print(type(a())) -- no error, there was 1 argument but was nil
Not only is it for readability, but it’s good practice to return nil to prevent edge cases like these.
. Basically, everytime a listener is fired, the