Feedback on this Tool Class System?

Help and Feedback Code Review
1

This is my first time using OOP, so I need some feedback to see if there’s anything that needs to be changed. This is a server and client script, which handles both the server and client, for better replication.

local ToolClass = {} -- Programmer : Galicate --// SERVICES //-- local PLAYER_SERVICE = game:GetService("Players") local REPLICATED_STORAGE = game:GetService("ReplicatedStorage") local RUN_SERVICE = game:GetService("RunService") local DEBRIS = game:GetService("Debris") --// METATABLES //-- ToolClass.__index = ToolClass --// CONSTANTS //-- local RemoteInstance = REPLICATED_STORAGE:WaitForChild("RemoteInstances") local ToolEvent = RemoteInstance:WaitForChild("toolEvent") local EventHandler = require(REPLICATED_STORAGE:WaitForChild("EventHandler")) local ItemDataFolder = REPLICATED_STORAGE:WaitForChild("ItemData") local LocalPlayer = PLAYER_SERVICE.LocalPlayer or nil --// VARIABLES //-- local toolClasses = {} local toolCache = {} --// LOCAL FUNCTIONS //-- local function init(...)	--// Load All Sub Class Tool Modules Into toolClasses	local subClasses = script:GetChildren()	for i, v in pairs(subClasses) do	if not v:IsA("ModuleScript") then continue end	toolClasses[v.Name] = require(v)	end	return end local function createEvent(self, eventName)	local newEvent = EventHandler.Event.new(eventName)	newEvent:Connect(function(...)	if RUN_SERVICE:IsServer() then	--// Fire To All Clients	ToolEvent:FireAllClients(self.UID, eventName, self.Owner, self.ToolId, self.Model)	elseif RUN_SERVICE:IsClient() and self.Owner == LocalPlayer then	--// Fire To Server	ToolEvent:FireServer(self.UID, eventName, self.Owner, self.ToolId, self.Model)	end	end)	return newEvent end local function onServerEvent(player, ...)	for i, v in pairs(PLAYER_SERVICE:GetPlayers()) do	if v ~= player then	--// Fire Tool Event To Client	ToolEvent:FireClient(v, ...)	end	end end local function onClientEvent(UID, eventName, playerInstance, toolId, toolModel)	if eventName == "Created" then	ToolClass.new(playerInstance, toolId, UID, toolModel)	end	local tool = toolCache[UID]	if tool then	if eventName == "Equipped" then	tool:Equip()	elseif eventName == "Unequipped" then	tool:Unequip()	elseif eventName == "Activated" then	tool:Activate()	elseif eventName == "Deactivated" then	tool:Deactivate()	elseif eventName == "Destroyed" then	tool:Destroy()	end	else	warn("Cache[" .. tostring(UID) .. "] is missing or nil.")	end end local function rollTrack(Tracks)	return Tracks[math.random(#Tracks)] end --// GLOBAL FUNCTIONS //-- function ToolClass.new(playerInstance, toolId, UID, toolModel)	--// Check For Errors	if not playerInstance then warn("Arg 1 nil") return end	if not toolClasses[toolId] then warn("Arg 2 nil") return end	if not UID then warn("Arg 3 nil") return end	if not toolModel then warn("Arg 4 nil") return end	local self = toolClasses[toolId].new(playerInstance, UID)	setmetatable(self, ToolClass)	--// Set Default Parameters	self.Owner = playerInstance	self.ToolId = toolId	self.UID = UID	self.Model = toolModel	--// Create Tool Parameters	self.ItemData = require(ItemDataFolder:FindFirstChild(toolId)) or {}	self.IsEquipped = false	self.Active = false	--// Create Events	self.Created = createEvent(self, "Created")	self.Equipped = createEvent(self, "Equipped")	self.Unequipped = createEvent(self, "Unequipped")	self.Activated = createEvent(self, "Activated")	self.Deactivated = createEvent(self, "Deactivated")	self.Destroyed = createEvent(self, "Destroyed")	--// Add Tool To Cache	toolCache[self.UID] = self	--// Fire Created Event	self.Created:Fire(self)	if toolModel then	toolModel:GetPropertyChangedSignal("Parent"):Connect(function()	if toolModel.Parent == nil then	self:Destroy()	end	end)	end	return self end function ToolClass:Equip()	if self.Owner then	self:UnloadAnimations()	end	self.Equipped:Fire()	self.IsEquipped = true	self:LoadAnimations()	warn(self.Owner.Name .. " has equipped " .. self.Model.Name)	toolClasses[self.ToolId].Equip(self) end function ToolClass:Unequip()	self.Unequipped:Fire()	self:Deactivate()	self.IsEquipped = false	warn(self.Owner.Name .. " has unequipped " .. self.Model.Name)	toolClasses[self.ToolId].Unequip(self) end function ToolClass:Activate()	if not self.IsEquipped then return end	self.Active = true	self.Activated:Fire()	warn(self.Owner.Name .. " has activated " .. self.Model.Name)	toolClasses[self.ToolId].Activate(self) end function ToolClass:Deactivate()	self.Active = false	self.Deactivated:Fire()	warn(self.Owner.Name .. " has deactivated " .. self.Model.Name)	toolClasses[self.ToolId].Deactivate(self) end function ToolClass:InputBegan(userInput)	if not self.IsEquipped then return end	warn(" [" .. self.Owner.Name .. "].InputBegan = " .. tostring(userInput)) end function ToolClass:InputEnded(userInput)	if not self.IsEquipped then return end	warn(" [" .. self.Owner.Name .. "].InputEnded = " .. tostring(userInput)) end function ToolClass:Destroy()	toolCache[self.UID] = nil	DEBRIS:AddItem(self.Model)	self.Destroyed:Fire()	toolClasses[self.ToolId].Destroy(self) end function ToolClass:LoadAnimations()	local animationController = self.Owner.Character.Humanoid:FindFirstChild("Animator")	if not animationController then return end	self.AnimationController = animationController	if self.Animations == nil then	local animations = {}	for i, v in pairs(self.ItemData.Animations) do	local tracks = {}	for i2, v2 in pairs(v) do	local animationObject = self.Model:FindFirstChild(i.."_"..tostring(i2))	if not animationObject then	animationObject = Instance.new("Animation")	animationObject.AnimationId = "rbxassetid://"..v2.Id	animationObject.Name = i.."_"..tostring(i2)	animationObject.Parent = self.Model	end	tracks[i2] = {	Track = animationController:LoadAnimation(animationObject),	Weight = v2.Weight,	AnimationObject = animationObject,	}	end	animations[i] = {	Tracks = tracks,	ActiveTrack = nil	}	end	self.Animations = animations	else	local AanimationController = self.Owner.Character.Humanoid:FindFirstChild("Animator")	for i, Data in pairs(self.Animations) do	for i2 = 1, #Data do	local TrackData = Data[i2]	if TrackData.Track then	TrackData.Track:Destroy()	end	TrackData.Track = animationController:LoadAnimation(TrackData.AnimationObject)	end	end	end end function ToolClass:UnloadAnimations()	if self.Animations then	for i, v in pairs(self.Animations) do	for TrackIndex = 1, #v.Tracks do	local TrackData = v.Tracks[TrackIndex]	if TrackData.Track then	TrackData.Track:Stop()	TrackData.Track:Destroy()	end	end	end	self.AnimationController = nil	end end function ToolClass:StopAnimation(animationName)	local animationData = self.Animations[animationName]	if animationData and animationData.ActiveTrack then	animationData.ActiveTrack:Stop()	if RUN_SERVICE:IsClient() and self.Owner == LocalPlayer then	ToolEvent:FireServer(self.ID, "StopAnimation", animationName)	elseif RUN_SERVICE:IsServer() then	ToolEvent:FireAllClients(self.ID, "StopAnimation", animationName)	end	else	warn("Warning: " .. self.ToolId .. ".Animations[" .. tostring(animationName) .. "] is missing or nil.")	end end function ToolClass:StopAllAnimations()	for i, v in pairs(self.Animations) do	if v and v.ActiveTrack then	v.ActiveTrack:Stop()	end	end	if RUN_SERVICE:IsClient() and self.Owner == LocalPlayer then	ToolEvent:FireServer(self.ID, "StopAllAnimations")	elseif RUN_SERVICE:IsServer() then	ToolEvent:FireAllClients(self.ID, "StopAllAnimations")	end end function ToolClass:PlayAnimation(animationName, isLooped)	local animationData = self.Animations[animationName]	if animationData then	local Data = rollTrack(animationData.Tracks)	animationData.ActiveTrack = Data.Track	animationData.Looped = isLooped or false	animationData.ActiveTrack:Play(nil, Data.Weight)	if RUN_SERVICE:IsClient() and self.Owner == LocalPlayer then	ToolEvent:FireServer(self.ID, "PlayAnimation", animationName)	elseif RUN_SERVICE:IsServer() then	ToolEvent:FireAllClients(self.ID, "PlayAnimation", animationName)	end	return animationData.ActiveTrack	else	warn("Warning: " .. self.ToolId .. ".Animations[" .. tostring(animationName) .. "] is missing or nil.")	end end --// EVENT CONNECTIONS //-- if RUN_SERVICE:IsServer() then	ToolEvent.OnServerEvent:Connect(onServerEvent) elseif RUN_SERVICE:IsClient() then	ToolEvent.OnClientEvent:Connect(onClientEvent) end --// INIT //-- init() return ToolClass
2

P.S: I feel like you just copy pasted the class from this post and modified it a bit.

4

I replied to the wrong person lol.

I think the functionality is fine because it does what it looks like you want it to do, but there are some small naming things I would change up about this. If you’re not worried about style, because it is subjective after all, feel free to disregard the rest of this.(btw all my advice comes from the Roblox Lua Style Guide)

Instead of having variables that use get Roblox services be notated as a constant (e.g MY_CONSTANT) I would use pascal case instead. Then I would change the actual constants to LOUD_SNAKE_CASE.

local UserInputService = game:GetService("UserInputService") local MY_CONSTANT = 10

Another change I would make stylistically is to change ToolClass to just Tool or PlayerTool, something along those lines. This is because a module that’s being used as a class usually doesn’t need Class at the end of the name. Although, it wouldn’t hurt keeping class for explicitness. Also, instead of playerInstance you can change it to player: Player. When you try to use the constructor in another file it’ll show that the parameter’s type is supposed to be Roblox’s Player type. I could go on about really small style things but it’s really up to you and it won’t matter if no one else is going to use it.

Cheers.

1 Like