Feedback on my DataObject class

Help and Feedback Code Review
, ,
#1

So I made this (1.2 KB) data object class as part of my data service for a game I’m in the process of developing. I’m wondering if there’s anything I should improve with it / if I’ve overlooked something. With this needing to support live data I want it to be bulletproof.

The idea is basically to have a more module friendly / lower footprint version of the NumberValue / StringValue etc. type instances that Roblox provide. What I like about these objects is that they fire an event when they are changed so it can be quite easy to make event based scripts using them.

I suppose one way to improve what I have is to add some sort of key based changed event rather than just the whole table, so any thoughts on how best to do that would be appreciated as well.

Feel free to use this code in your own projects as well if you think you could get some use out of it


local dataObject = {}

local function CopyTable(t)
	assert(type(t) == "table", "First argument must be a table")
	local tCopy = table.create(#t)
	for k,v in pairs(t) do
		if (type(v) == "table") then
			tCopy[k] = CopyTable(v)
		else
			tCopy[k] = v
		end
	end
	return tCopy
end

function dataObject.new(tab)
	
	if tab then	
		for key, value in pairs(tab) do
			assert(key ~= "_event", "Cannot have \"_event\" as a key.", debug.traceback())
			assert(key ~= "Changed", "Cannot have \"Changed\" as a key.", debug.traceback())
		end
	end
	
	local newObj = setmetatable(tab and CopyTable(tab) or {}, dataObject)
	
	newObj._event = Instance.new("BindableEvent")
	newObj.Changed = newObj._event.Event
	
	return newObj
end

function dataObject:Get(key)
	return self[key]
end

function dataObject:Set(key, value)
	self[key] = value
	self._event:Fire(key, value)
end

function dataObject:Update(key, func)
	local newValue = func(self[key])
	self:Set(key, newValue)
end

function dataObject:Delete(key)
	self[key] = nil
end

function dataObject:Destroy()
	
	self._event:Destroy()
	self = nil
	
end

dataObject.__index = dataObject

return dataObject
1 Like
#2
  • CopyTable does not maintain table order (pairs iterates in arbitrary order, might not be too important)
  • In dataObject.new perhaps you are better off creating an array of reserved keys and doing a loop to check if a reserved key is being used
  • I don’t like the parameter name for CopyTable and dataObject.new. The inconsistency is strange. Both are tables, so why are they named differently? Also the name tab confused me at first.

Looks good overall. The only thing I didn’t like were the random empty lines at dataObject:Destroy and the start of dataObject.new but that is probably highly opinionated.

1 Like
#4

I like your point about making use of arrays being faster to loop through numerically. I’m curious to see how much of a difference this would make, have you done much testing on it?

With checking the keys I don’t see the need to do it for the setter function. I was just planning on using the set function to initialise stored key, value pairs.

For the get function I figured if there’s no value corresponding to the key the function will just return nil. Perhaps that could make debugging harder so might be good to have a warning in there.

#5

In the current state of Luau, the difference between ipairs and pairs is trivial in terms of performance. Rather, you should use ipairs to denote that only consecutive integer keys starting with 1 are to be used. Everything else, especially in cases where a table’s shape is not known, should use pairs: