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