What do you guys think of my OOP data module?

Help and Feedback Code Review
#1

I just finished my OOP data module and it saves values fine but am wondering if there is a better way to do it then the way im doing?

local DataService = {}

local IndexTable = {
	__index = DataService
}

local MainStore = game:GetService("DataStoreService"):GetDataStore(script:WaitForChild("DataKey").Value)

local RunService = game:GetService("RunService")

--Function Used To Get The Value Type
local function GetDataType(data)
	if typeof(data) == "number" then
		return "IntValue"
	elseif typeof(data) == "string" then
		return "StringValue"
	elseif typeof(data) == "boolean" then
		return "BoolValue"
	end
end

--Data Object Constructer
function DataService.new(UserId)
	local newData = {}
	setmetatable(newData, IndexTable)
	
	--Makes the server sided folder to store the real data in
	local PlayerDataFolder = Instance.new("Folder")
	PlayerDataFolder.Parent = game:GetService("ServerStorage"):WaitForChild("GlobalData")
	PlayerDataFolder.Name = "Data-"..UserId
	
	--creates a client sided folder to locally store the data for access from UIs and stuff
	local PlayerLocalFolder = Instance.new("Folder")
	PlayerLocalFolder.Parent =  game:GetService("ReplicatedStorage"):WaitForChild("LocalData")
	PlayerLocalFolder.Name = "Data-"..UserId	
	
	newData.PlayerId = UserId
	newData.Key = "Key-"..UserId
	newData.GlobalData = PlayerDataFolder
	newData.LocalData = PlayerLocalFolder

	--adds player removing function inside automatically
	game.Players.PlayerRemoving:Connect(function(player)
		if player.UserId == UserId then
			DataService:SaveData(player.UserId)
		end
	end)
	
	return newData
end

function DataService:CreateNewData(ValueName, DefaultValue)
	--creates new value that will be server sided
	local ValueObjGlobal = Instance.new(GetDataType(DefaultValue))
	ValueObjGlobal.Parent = self.GlobalData
	ValueObjGlobal.Value = DefaultValue
	ValueObjGlobal.Name = ValueName
	
	--crates new local value to be used to be indexed by UIs and stuff
	local ValueObjLocal = Instance.new(GetDataType(DefaultValue))
	ValueObjLocal.Parent = self.LocalData
	ValueObjLocal.Value = ValueObjGlobal.Value
	ValueObjLocal.Name = ValueObjGlobal.Name
	
	--updates the local value anytime the server value changes 
	ValueObjLocal.Value = ValueObjGlobal.Value
	ValueObjGlobal.Changed:Connect(function()
		ValueObjLocal.Value = ValueObjGlobal.Value
	end)
	
	local data
	
	local success, err = pcall(function()
		data = MainStore:GetAsync(self.Key)
		
		--loops through the data and sees if it can find a key with the value name, if not the value is default
		
		if data then
			for _,v in pairs(self.GlobalData:GetChildren()) do
				if data[v.Name] ~= nil then
					v.Value = data[v.Name]
				end
			end
		end
	end)
	
	if not success then
		print("error loading data: ")
		print(err)
	end
end

function DataService:SaveData(id)
	local SaveTable = {}

	for _,v in pairs(game:GetService("ServerStorage"):WaitForChild("GlobalData")["Data-"..id]:GetChildren()) do
		SaveTable[v.Name] = v.Value
	end
	
	--inserts the data values into the table with there naeme as a key
	
	--removes any gaps in the table
	for k,v in pairs(SaveTable) do
		if v == nil then
			table.remove(SaveTable, k)
		end
	end
	
	--set the value
	local success, err = pcall(function()
		MainStore:SetAsync("Key-"..id, SaveTable)
	end)
			
	if not success then
		print("error saving data")
		print(err)
	end
end

--added bind to close in order to prevent data loss from server shutdown
game:BindToClose(function()
	if RunService:IsStudio() then
		--to make the data saving in studio more reliable
		wait(3)
	else
		for _,v in pairs(game.Players:GetPlayers()) do
			DataService:SaveData(v.UserId)
		end
	end
end)
	

return DataService
#2

You should wrap your saving and getting into a repeat call because once your pcall for any of those requests failed, you don’t retry.

if RunService:IsStudio() then
		--to make the data saving in studio more reliable
		wait(3)

There is no need of a wait, just use return.

for _,v in pairs(game.Players:GetPlayers()) do
			DataService:SaveData(v.UserId)
		end

I’d advise changing v to player since you are looping through all the player’s one by one and it makes more sense therefore.

if not success then
		print("error saving data")
		print(err)
	end

Instead of printing err after error saving data since it looks more clean like that.

if not success then
		print("error saving data: " .. err)
	end

	if data then
			for _,v in pairs(self.GlobalData:GetChildren()) do
				if data[v.Name] ~= nil then
					v.Value = data[v.Name]
				end
			end
		end
	end)

Instead of checking if it isn’t nil, you can use if statements like:

if data[v.Name]  then

Same thing, consider this a negligible micro optimization. Use ipairs when looping through arrays and pairs when looping through dictionaries.

#3

Thanks for the advice :grin: The data[v.Name] I have to keep though since the data values could be false such as if they own a weapon.

1 Like
#4

One thing I’d like to know is why are you using metatables in your data module?

local newData = {}

You would only use if you to tried index something that is nil in newData but you don’t do anything to replace the value that you tried to indexed which was nil.

#5

I am using meta tables to give it OOP functionality so each player has there own data that I can manipulate easier