Did I over complicate this ToolHandling system?

[WizulousThe2nd]

The ToolHandling System is made up of

• ToolClass
• ToolHandler
• 1 Data, 1 Module for each Tool added (Rn there is one tool)

I would appreciate any feedback and advice on this. This is my 2nd time using a Class Module.
I genuinely would just like to improve this system and learn the use-case for class modules.

TOOLHANDLER:

local module = {}

--SERVICES		
local ReplicatedStorage = game:GetService("ReplicatedStorage")	
local Players = game:GetService("Players")	
local Teams = game:GetService("Teams")

--VARIABLES	
local ModuleLoader = require(Players.LocalPlayer.PlayerScripts.Client.ModuleLoader)	
local ToolClass = require(Players.LocalPlayer.PlayerScripts.Client.Modules.Classes.ToolClass)	

--PRIVATE VARIABLES		
local LocalPlayer = Players.LocalPlayer	
local RegisteredTools = {}

--PRIVATE FUNCTIONS		
local function RegisterTool(Tool: Tool, Humanoid: Humanoid)	
	for _, MetaTable in pairs(RegisteredTools) do
		if MetaTable.Tool == Tool then	
			return
		end
	end		
	
	local ToolObject = ToolClass.New(Tool)		
	table.insert(RegisteredTools, ToolObject)		
	
	ToolObject.Tool.Equipped:Connect(function() ToolObject:Equipped(Humanoid) end)
	ToolObject.Tool.Unequipped:Connect(function() ToolObject:Unequipped(Humanoid) end)
end

--PUBLIC FUNCTIONS				
function module.Start()	
	LocalPlayer.CharacterAdded:Connect(function(Character)	
		local Humanoid = Character:WaitForChild("Humanoid")			
			
		for _, MetaTable in pairs(RegisteredTools) do
			MetaTable:Destroy()
		end			
		
		RegisteredTools = {}

		Humanoid.Died:Connect(function()	
			for _, MetaTable in pairs(RegisteredTools) do
				MetaTable:Unequipped(Humanoid)
			end	
		end)

		for _, Tool in pairs(LocalPlayer.Backpack:GetChildren()) do
			if Tool:IsA("Tool") and Humanoid.Health ~= 0 then	
				RegisterTool(Tool, Humanoid)
			end
		end	

		LocalPlayer.Backpack.ChildAdded:Connect(function(Tool)	
			if Tool:IsA("Tool") and Humanoid.Health ~= 0 then	
				RegisterTool(Tool, Humanoid)
			end
		end)	
	end)
end

return module

TOOLCLASS

local module = {}	
module.__index = module

--SERVICES		
local ReplicatedStorage = game:GetService("ReplicatedStorage")	
local Players = game:GetService("Players")	
local Teams = game:GetService("Teams")

--VARIABLES	
local ModuleLoader = require(Players.LocalPlayer.PlayerScripts.Client.ModuleLoader)	

--PRIVATE VARIABLES		
local LocalPlayer = Players.LocalPlayer		
local DataModules = {}

--PRIVATE FUNCTIONS		

--PUBLIC FUNCTIONS					
function module.Start()	
	for _, DataModule in ipairs(script.Parent.Parent.Handlers.ToolHandler.ToolData:GetChildren()) do
		if DataModule:IsA("ModuleScript") then		
			DataModules[DataModule.Name] = require(DataModule)
		end	
	end
end

function module.New(Tool: Tool)	
	for DataName, DataModule in pairs(DataModules) do	
		if DataName == Tool.Name then		
			local self = setmetatable({}, module)	
			self.Tool = Tool	
			self.ToolModule = DataModule.ToolModule	
			self.ToolAnimationInfo = DataModule.ToolAnimationInfo
			return self
		end
	end
end		

function module:Equipped(Humanoid: Humanoid)	
	if self.Tool and Humanoid.Health ~= 0 then	
		self.ToolModule.EquippedFunction(Humanoid)
	end
end	

function module:Unequipped(Humanoid: Humanoid)	
	self.ToolModule.UnequippedFunction(Humanoid)
end

function module:Destroy()	
	setmetatable(self, nil)	
	table.clear(self)
end

return module

Example: ToolData Module / ToolFunction Module

return {	
	ToolName = "ID Card",	
	ToolModule = require(script.Parent.Parent.ToolFunctions["ID Card"]),	
	ToolAnimationInfo = {}
}

local module = {}

--SERVICES		
local ReplicatedStorage = game:GetService("ReplicatedStorage")	
local Players = game:GetService("Players")	
local Teams = game:GetService("Teams")

-- VARIABLES	
local ModuleLoader = require(Players.LocalPlayer.PlayerScripts.Client.ModuleLoader)	
local IDCardEvent = ReplicatedStorage.Network.ToolEvents.IDCard

--PRIVATE VARIABLES			
local LocalPlayer = Players.LocalPlayer		
local Equipped = false	

--PRIVATE FUNCTIONS		

--PUBLIC FUNCTIONS				
function module.EquippedFunction(Humanoid)	
	Equipped = true	
	IDCardEvent:FireServer(Equipped)
end

function module.UnequippedFunction(Humanoid)	
	if Equipped == true then	
		Equipped = false
		
	end
end

return module

[roalex2008 (Dottik)]

Let me begin by the following. I HIGHLY recommend you to not name a class “ToolClass”, it makes little sense, I’d name it just CustomTool or so to not collide with the already existing Roblox Tool instance class.

From what I see your RegisterTool function wraps around the given tool into a ToolClass.

local function RegisterTool(Tool: Tool, Humanoid: Humanoid)	
	for _, MetaTable in pairs(RegisteredTools) do
		if MetaTable.Tool == Tool then	
			return
		end
	end		
	
	local ToolObject = ToolClass.New(Tool)		
	table.insert(RegisteredTools, ToolObject)		
	
	ToolObject.Tool.Equipped:Connect(function() ToolObject:Equipped(Humanoid) end)
	ToolObject.Tool.Unequipped:Connect(function() ToolObject:Unequipped(Humanoid) end)
end

This snippet might lead to weird behaviour in some cases, mainly in the point that Humanoid is not something obtained from, for example, a Player object, it would be a reference, and if a player dies, it would no longer be valid, but at that point, you would have already lost the tool. Besides that, I would recommend you to, when the tool is destroyed (Tool.Destroying) to connect it and to remove it from RegisteredTools in a safe manner, with a mutex of sorts, as Lua is all serial, not parallel, it will work alright, like the following

-- ...
mutx = false -- Global so it can be shared between other Removing events, else move it into a higher lexical scope..
tool.Tool.Removing:Connect(function()
    repeat task.wait(0.03) until not mutx 
    mutx = true
    table.remove(toolInstnaces, table.find(toolInstnaces, tool))
    mutx = false
end)

Besides that, I would rename the ToolHandler to ToolManager. As it manages them.

That being said, I would recommend you to make the ToolClass class more of a base class, which you require, and you base off from to provide a better experience, being that if you want to extend it, it would be a simple

local ReplicatedStorage = game:GetService("ReplicatedStorage")

local toolBaseFactory = require(...)  -- ToolBaseFactory { new: () -> ToolBase { OnEquipped: () -> (), 
local IDCardEvent: RemoteEvent = ReplicatedStorage:WaitForChild("Network"):WaitForChild("ToolEvents"):WaitForChild("IDCard")
OnUnequipped: () -> (), IsEquipped: () -> boolean, ToolName: string }  }
local nTool = toolBaseFactory.new()

nTool.ToolName = "KeyCard"
nTool.OnUnequipped = function()
    print("I have been unequipped!")
end
nTool.OnEquipped = function()
    print("I have been equipped!")
    IDCardEvent:FireServer(nTool.IsEquipped())
end

return nTool

This is a sample with somewhat equal behaviour to that of the original, not the best, but you can make out an idea on my opinion here, still, your way of achieving it is alright, this is just the way I’d do it personally.

As for if it is complicated? I’d argue it sort of is, but is it over complicated? That is another part of discussion, becuase if we go down that rabbit hole we would have to name everything like Knit, Nevermore, etc Overcomplicated, because that is what they are.

[WizulousThe2nd]

Thank you so much for this , just two questions…

What is a base class, and is this code a type of alternative that eliminates the need for the ToolHandler? (I plan on renaming it Manager)

@roalex2008

That being said, I would recommend you to make the ToolClass class more of a base class, which you require, and you base off from to provide a better experience, being that if you want to extend it, it would be a simple

local ReplicatedStorage = game:GetService("ReplicatedStorage")

local toolBaseFactory = require(...)  -- ToolBaseFactory { new: () -> ToolBase { OnEquipped: () -> (), 
local IDCardEvent: RemoteEvent = ReplicatedStorage:WaitForChild("Network"):WaitForChild("ToolEvents"):WaitForChild("IDCard")
OnUnequipped: () -> (), IsEquipped: () -> boolean, ToolName: string }  }
local nTool = toolBaseFactory.new()

nTool.ToolName = "KeyCard"
nTool.OnUnequipped = function()
    print("I have been unequipped!")
end
nTool.OnEquipped = function()
    print("I have been equipped!")
    IDCardEvent:FireServer(nTool.IsEquipped())
end

return nTool

[roalex2008 (Dottik)]

Think of the manager as your way of interacting with it. You want to keep it to control all of the Tools.

A base class is basically a class which everything if you will originates from, it is an Inheritance pattern, one I don’t particularly like due to its nature of making code complex to work with, as behaviour can be overriden and not overriden. This way you can maintain the base implementation, but extend it if you need it. I would normally gravitate towards composition, but I’m yet to develop a pattern to the likes of it on Roblox, I used Inheritance in some of my projects, particularly my experimental game framework VEngine, in it, I use the same basis, or a similar one.

The framework is designed with modules, as normal.

This is the base module EngineModule, as you can see, its a factory that makes new modules, the reason for it to be a factory is because I didn’t want to wrap my head around the fact that require caches returns, so modifying it could lead to unexpected behaviour, by making it a factory I avoided that possibility outright, that aside, You see that it contains a namecall “Initialize”, that you can override, which is effectively overriden inside of Client/HelloWorld the HelloWorld example modules. It requires it, makes a new one, and goes from it.

I merely use it to uphold the contract, which is fancy wording for saying “This table will have AT LEAST this members!”, (Initialize, ModuleName). I recommend you to use Roblox LSP if you want to understand VEngine, because it relies on a lot of it for its magic, using Roblox LSP I can access the members, and it makes the type unsafe Luau, less type unsafe, if you will.

To sum up, though, think of a Base Class as your blueprint, and everything you make from it is a new variant of it, it will follow its base rules (Structure), but it has extra things in it.