Debris2 | Need reviews

Help and Feedback Code Review
#1

Before releasing Debris2 to the Community as an Open Source Module, I’d like to make sure that this Code is of highest quality possible in terms of readability & efficiency

Current features:

  • Canceling
  • Usage with Non Instance (RBLXScriptConnection and tables)
  • Adding an Array of Items instead of just one at a time
  • Overriding lifeTime
  • A Callback for when the Item has been Destroyed
Source Code 
--[[ 
	@game/ReplicatedStorage/Debris2
		Debris2;
--]]

local Debris2 = {
	Instances = {}
}

--| SERVICES:
local RunS = game:GetService("RunService")
	local Heatbeat = RunS.Heartbeat

--| MODULES:

--| VARIABLES:

local Instances = Debris2.Instances

--| TABLES:

local Connections = {}

local ValidTypes = {
	["Instance"] = "Destroy",
	["table"] = true,
	["RBXScriptConnection"] = "Disconnect",
}

local METHODS = { -- add any Custom Destroy/Remove/Clear/CleanUp methods here
	"Destroy",
	"Disconnect",
	
	"destroy",
	"disconnect",
}

--| META TABLES:

--| FUNCTIONS:

local function removeItem(typeOf,object)
	if typeOf == "Instance" then
		pcall(object.Destroy,object)
	elseif typeOf == "RBXScriptConnection" then
		pcall(object.Disconnect,object)
	else
		for _,v in ipairs(METHODS) do -- _, v: method name
			if object[v] then
				pcall(object[v], v)
				break
			end
		end
	end
end

local function addDebris(object,lifeTime)
	
	local typeOf = typeof(object)
	
	assert(ValidTypes[typeof(object)])
	assert(typeof(lifeTime) == "number")
	
	if (not Instances[object]) then
		table.insert(Instances,object)
	end
	
	Instances[object] = {
		["lifeTime"] = lifeTime,
		removalTime = tick() + lifeTime,
--		Destroyed = nil, -- Destroyed: callback Function
		Cancel = function() -- remove references and disconnect Hearbeat
			Connections[object]:Disconnect()
			table.remove(Instances,table.find(Instances,object))
			Instances[object] = nil
		end,
		["Instance"] = object,
	}
	
	local debris = Instances[object]
	
	Connections[object] = Heatbeat:Connect(function()
		if debris and tick() >= debris.removalTime then
			if debris.Destroyed then
				debris.Destroyed()
			end
			debris.Cancel()
			removeItem(typeOf,object)
		end
		debris = Instances[object]
	end)
	
	return Instances[object]
end

--| METHODS:

function Debris2:AddItem(item,lifeTime) -- item: (Instance, table, RBXScriptConnection), lifeTime: number
	return addDebris(item,lifeTime)
end

function Debris2:AddItems(arrayOfItems,lifeTime) -- arrayOfItems: (Instance, table, RBXScriptConnection), lifeTime: number
	for _,v in ipairs(arrayOfItems) do -- _, v: Instance
		addDebris(v,lifeTime)
	end
end

function Debris2:GetAllDebris()
	return Instances
end
Debris2.getAllDebris = Debris2.GetAllDebris

function Debris2:GetDebris(instance)
	return Instances[instance]
end
Debris2.getDebris = Debris2.GetDebris

--| SCRIPTS:

-- return:
return Debris2

Pastebin

  • Ready to ship
  • Needs improvements (reply with improvement!)

0 voters

I probably will need help with Documentation & Github, as this is my first Open Source project

2 Likes
Debris2 | Open Source
#2

self isn’t used in these functions, so they should be defined using a dot . with no self argument.
Doing this would also make Debris2.AddItem = addDebris
GetAllDebris seems weird, it’s just returning a single constant value. It would make more sense for Debris2.AllDebris = Instances.

1 Like
#3

I use : to mimic the original method, you can call it with either . Or : doesn’t really matter which one it’s using, thus that’s not an issue.

You can also do Debris2.Instances however I added a get function so it’s simple to use.

that should explain the decisions made while I was writing this Module.

#4

It seems like your code is pretty inconsistent. I’d say pick a style and stick to it.

Also, like @Halalaluyafail3 said, functions declared with the implicit self argument that never uses self should not be thought of as instance methods, but rather static functions. As such, this should be addressed.

While, yes, this functionally is correct it violates the intent to the end-user. This should always be avoided.

#5

Can you point this out to be more precise?

#6

Sure thing!

  • Your vertical spacing is inconsistent.
  • Sometimes spaces come after commas, sometimes they don’t. Spacing between operators and delimiters should be consistent.
  • Sometimes you use property field syntax for table fields, sometimes you use computed properties for table fields that are string literals needlessly. I advise you use the property syntax over the element syntax.
  • Sometimes you make conditional expressions parenthesized, sometimes you don’t. I advise that you always don’t unless if the expression itself spans multiple lines (which, in your case, it does not).
  • Sometimes you use camelCase or sometimes you use PascalCase within the same scope (most noticeably in local variables that are not representative of a service, module, class-like construct, or something similar)

These are just the main things I noticed when skimming through the source.

#7

Fixed

Usually, I use it to wrap a not operator or separate conditions

Fixed

I don’t want

["lifeTime"] = lifeTime,
["Instance"] = object,

to be confusing