How's this task object?

Help and Feedback Code Review
#1

I’m gonna have a service for creating tasks when a player asks for them, and there will be presets available, but for now you can create a blank task and objective, and then just add the objectives you want to the task and then activate it.

Task

-- TaskClass.lua
-- Galicate @ 11:09 12/10/2024

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Signal = require(ReplicatedStorage.Util.Signal)

local TaskClass = {}
TaskClass.__index = TaskClass

-- Returns a new <code>Task</code> class
function TaskClass.new(title: string?)
	local self = setmetatable({}, TaskClass)
	
	self.Title = title or "Task"
	self.IsActive = false
	self.IsCompleted = false
	self.IsCancelled = false
	self.Players = {}
	self.Participants = {} -- Used for multi-player tasks for checking who has actually contributed something to earn rewards.
	self.Objectives = {}
	self.Events = {
		Activated = Signal.new(),
		Completed = Signal.new(),
		Cancelled = Signal.new(),
	}

	return self
end

-- Returns whether the task is currently active
function TaskClass:IsActive(): boolean
	return self.IsActive
end

-- Returns whether the task is completed
function TaskClass:IsCompleted(): boolean
	return self.IsCompleted
end

-- Returns a table of all players assigned to the task
function TaskClass:GetPlayers(): {Player}
	return self.Players
end

-- Returns a table of all objectives of this task
function TaskClass:GetObjectives(): {}
	return self.Objectives
end

-- Adds a player/players to the table of players assigned to the task
function TaskClass:AddPlayers(players: Player | {Player}): {Player}
	if typeof(players) == "Player" then
		table.insert(self.Players, players)
	elseif typeof(players) == "table" then
		for _, player in pairs(players) do
			table.insert(self.Players, player)
		end
	end
	if #self:GetPlayers() < 1 then self:Destroy() end
	return self.Players
end

-- Removes a player/players from the table of players assigned to the task
function TaskClass:RemovePlayers(players: Player | {Player}): {Player}
	if typeof(players) == "Player" then
		for index, player in pairs(self.Players) do
			if player == players then
				table.remove(self.Players, index)
			end
		end
	elseif typeof(players) == "table" then
		for _, player in pairs(players) do
			for index, _player in pairs(self.Players) do
				if player == _player then
					table.remove(self.Players, index)
				end
			end
		end
	end
	return self.Players
end

function TaskClass:AddObjective(objective: {Parent: {any}, IsActive: boolean, IsCompleted: boolean})
	objective.Parent = self
	table.insert(self.Objectives, objective)
end

--[[
	Activates the task and all of its assigned objectives
	It is recommended to activate AFTER all objectives have been added to the task
]]
function TaskClass:Activate()
	if self:IsActive() then error("This task is already active") end
	self.IsActive = true
	self.Events.Activated:Fire()
end

--[[
	Bypasses the objective completion check and completes the task, used by objective class to complete the objective
	This should only be used in extreme cases where the objective completion check is not working correctly
]]
function TaskClass:Complete(bypassRequirements: boolean?)
	if self:IsCompleted() then error("This task is already completed") end
	if not bypassRequirements and not self:CanComplete() then warn("This task has remaining objectives to complete") end
	self.IsCompleted = true
	self.Events.Completed:Fire()
end

--[[
	Used by `ObjectiveClass` to determine if it should call `:Complete()`.
	Returns `true` if all objectives are completed and the task is not cancelled.
	Returns `false` otherwise
]]
function TaskClass:CanComplete(): boolean
	if self.IsCancelled then return false end
	for _, objective in pairs(self:GetObjectives()) do
		if not objective:IsCompleted() then
			return false
		end
	end
	return true
end

--[[
	Cancels the task making it impossible to complete
	Used for fail states or cancelling a task early, such as if a player dies while doing a task
]]
function TaskClass:Cancel()
	if self:IsCompleted() then error("This task is already completed") end
	self.IsCancelled = true
	self.Events.Cancelled:Fire()
end

function TaskClass:Destroy()
	for _, objective in pairs(self:GetObjectives()) do
		objective:Destroy()
	end
	for _, event: RBXScriptConnection in pairs(self.Events) do
		event:Disconnect()
	end
	self = nil
end

return TaskClass

Objective

-- ObjectiveClass.lua
-- Galicate @ 12:00 12/11/2024

local ObjectiveClass = {}
ObjectiveClass.__index = ObjectiveClass

-- Returns a new <code>Objective</code> class
function ObjectiveClass.new()
	local self = setmetatable({}, ObjectiveClass)

	self.IsActive = false
	self.IsCompleted = false

	return self
end

-- Returns whether the objective is currently active
function ObjectiveClass:IsActive(): boolean
	return self.IsActive
end

-- Returns whether the objective is completed
function ObjectiveClass:IsCompleted(): boolean
	return self.IsCompleted
end

function ObjectiveClass:Activate()
	if self:IsActive() then error("This objective is already active") end
	self.IsActive = true
end

function ObjectiveClass:Complete()
	if self:IsCompleted() then error("This objective is already active") end
	self.IsCompleted = true
	if self.Task:CanComplete() then
		self.Task:Complete()
	end
end

function ObjectiveClass:Destroy()
	self = nil
end

return ObjectiveClass
2 Likes
#2

Obviously, writing accessor and mutators is beneficial in other languages. But when you can’t modify a variables visibility, something like a mutator is redundant unless you’re looking to format the data in some specific way.

Setting self=nil in the Destroy method is redundant as well. self is a pointer to TaskClass in the method scope. Because there is no dereference operator in roblox, you cant actually access the “TaskClass” table itself. Only its contents. nullifying self just removes the pointer, which gets gcd and reallocated anyway when that method terminates.
following test confirms this:

local o = require(script.Parent:WaitForChild("ModuleScript")).new()
print(o)
o:test()
print(o)

local module = {}
module.__index = module

function module.new()
	return setmetatable({a=1,b=2},{__index=module})
end

function module:test()
	self = nil
end

return module

▼ {
[“a”] = 1,
[“b”] = 2
} - Client - LocalScript:2
02:36:41.829 ▼ {
[“a”] = 1,
[“b”] = 2
} - Client - LocalScript:4

Best (and only) way to delete “objects”, which remember are really just tables is to nullify the variable holding the table, and all pointers to that table.

Otherwise, seems very a very solid implementation of your idea. Well organized and well thought out. S tier readability.

3 Likes