Instance.old code review (metatables)

Help and Feedback Code Review
#1

I recently made this community resource:

I haven’t really messed around with metatables much.
Anyone wanna give me tips on this code?

Source: https://pastebin.com/raw/yGx4Awzg

I’ve basically made a wrapper around instance objects to override the default :Destroy() function. Instead of destroying the instance, it removes all event connections and adds the instance to a folder where it eventually gets recycled.

#2

Overall, the quality of your code is fairly decent. A few things to point out though:

You technically did not wrap the object yielded by Instance.new. All you did was return a dictionary binded to a few metamethods. An actual wrapper would return the primitive datatype back that was inputted whilst providing the extra functionality or by overriding the methods.

ex.

-- A chunk of a simple table wrapper I made a few months ago
local DictionaryFuncs = {}
local ArrayFuncs = {
   ConvertSetAndBag = function(self)
       local SetBagDST = {}
       for index, value in ipairs(self) do
          SetBagDST[value] = true
       end
       return SetBagDST
   end
}

local function OnIndexed(self, index)
   local Link = ( self[1] and ArrayFuncs[index] ) or DictionaryFuncs[index]
   if Link then
      return Link(self)
   end
   return false
end
local Metatable = {__index = OnIndexed}

return function(RequestedTable)
   return setmetatable(RequestedTable, Metatable)
end

-- Implementation
local TableWrapper = require('TableWrapper')

local Example = TableWrapper({'One', 'Two', 'Three'})
Example = Example.ConvertSetAndBag

for index, value in pairs(Example) do
   print(index, value)
end

^^ The table we input in our wrapper module is given back with the extra functionality in our ArrayFuncs and DictionaryFuncs tables via metamethods.

Good. I’ve seen a lot of scripters use metatables extraneously and end up over-engineering their code. In your scenario, you had a proper use-case for metatables. My main point here is to let you know in the future that metatables ~= automatically better lol.

Back to actually reviewing your code… :joy:

  • First thing I immediately see is your use of globals. It is advised on roblox docs that globals are bad practice.

“Local variables are obtained faster than global variables because they’re integrated into the environment in which they were created. If possible, you should always use local variables over global variables, unless there’s a specific reason otherwise.”
Source

A more in-depth explanation on why globals are slower to reference than local variables is because of the different DST they are stored in. Globals are stored on the heap, while local variables are accessed through the stack.

Instead of using globals here, there is a good work around:

local dprint
if Debug then
   dprint = function(...)
      warn(...)
   end
else
   dprint = function() end
end

  • Realistically, I feel having a custom debug function is pointless. It might be a good idea since you are open sourcing this but I doubt the person using this to want to get messages in his output that his instance was recycled or created. Unnecessary function calls would occur if the Debug variable is false.

  • Your addConnection subroutine doesn’t even have to be a subroutine. You are calling the subroutine in only one area of your code (in the __index metamethod). Normally I would make a function to follow the D.R.Y principle (Don’t Repeat Yourself) or to simply return a value that can’t be accessed in the current scope. You might be thinking “Well having this function looks neater”. Well the good thing, is that the user won’t have to worry about what the code looks like since it’s in a separate module providing the necessary abstraction.

  • You have a few extraneous / repetitive calculations:

  1. I would advise against calling rawget twice here to prevent the operation of blocking metamethod calls. I recommend doing this instead
local safeValue = rawget(fakeInst, index)
if safeValue then
   return safeValue
end
  1. You seem to index dictionary values a lot throughout your code without caching them in variables.
local realValue = realInstance[index] -- Use this value instead of always doing "realInstance[index]" repetitively
  • I’m a bit confused why you would use recursion here. This “createInstanceWrapper” is made to wrap instances not RBXScriptSignals. The recursion seems a little over-engineered.
			if typeof(realInstance[index])=="RBXScriptSignal" then -- FAKE/CUSTOM RBXScriptSignal
				return createInstanceWrapper(realInstance[index],{
					Connect = function(fake,...)
						local connection = realInstance[index]:Connect(...)
						addConnection(realInstance,connection)
						return connection
					end,
					Wait = function(fake,...)
						return realInstance[index]:Wait(...)
					end,
				},true)
			end

Overall, great script. Just has a few bad practices.
Happy Coding

2 Likes
#3

Oh that’s interesting. I didn’t know there was a classical definition of a wrapper.
It’s true that if the user did something like

print(typeof(Instance.old("Part")))

it would return “table” and not “Instance.”
Is there any way to really create a wrapper for an Instance?

Also, the recursion wasn’t really engineered at all, it was intuitive imo. I didn’t realize RBXScriptSignals weren’t considered Instances. I would’ve named the function more clearly if I did.

Thanks for the feedback!

1 Like
#4

I doubt there would be, since the metatable of any luau specific userdata is locked.
ex.

print(getmetatable(Instance.new('Part')))

^^ It will return a string saying that the metatable is locked. Meaning there is no way to alter it.

Why you can’t make a wrapper for an instance:

  • Metatable is locked through the __metatable metamethod
  • Instead of using a simple setmetatable, roblox uses a newproxy(true) userdata which is more private in a sense (raw functions don’t work on it, etc…)
print(type(Instance.new('Part'))) -- userdata
  • But it’s not completely impossible. Exploiters do have access to a function called getrawmetatable() which bypasses the security that instance objects have.
1 Like