Seeking program architecture advice for datastores

[Mystifine]

Greetings, I am currently working on a Datastore module, there are currently no problems whatsoever however I’m looking to improve the architecture of my program. Here’s the main module:

--|| Services ||--
local DataStoreService = game:GetService("DataStoreService");
local RunService = game:GetService("RunService");
local Players = game.Players;

local Datastore = {};
Datastore.__index = Datastore;
local Cache = {};
local StarterData = require(script.StarterData);
local Configs = require(script.Configurations);

--|| Local Functions
local Datatypes = {
	["number"] = "NumberValue",
	["string"] = "StringValue",
	["boolean"] = "BoolValue",
}

local function GetIndexCount(Table)
	local Count = 0;
	for _, _ in next, Table do
		Count += 1;
	end
	return Count;
end

local function Copy(Table)
	local NewTable = {}
	for Index, Value in next, Table do
		if typeof(Value) == "table" then
			NewTable[Index] = Copy(Value)
		else
			NewTable[Index] = Value
		end
	end
	return NewTable
end

local function GetSession()
	return {
		JobId = game.JobId;
		Stamp = os.time();
	};
end

local function GetTimeStamp()
	local Info = os.date("!*t")
	local TimeStamp = "DATE: "..Info.year.."/"..Info.month.."/"..Info.day.." | TIME: "..(Info.hour > 12 and Info.hour - 12 or Info.hour)..":"..(Info.min < 10 and "0"..Info.min or Info.min)..":"..(Info.sec < 10 and "0"..Info.sec or Info.sec).." "..((Info.hour < 12 or Info.hour == 24) and "AM" or "PM")
	return TimeStamp	
end

local function GetDataStore(Identifier)
	return DataStoreService:GetDataStore(tostring(Identifier));
end

local function GetOrderedDatastore(Identifier)
	return DataStoreService:GetOrderedDataStore(tostring(Identifier))
end

local function CreateDataTree(Parent, Data)
	for Index, Value in next, Data do
		local Datatype = type(Value);
		if Datatype == "table" then
			local Folder = Instance.new("Folder");
			Folder.Name = Index;
			CreateDataTree(Folder, Value);
			Folder.Parent = Parent;
		else
			local Object = Instance.new(Datatypes[Datatype]);
			Object.Value = Value;
			Object.Name = Index;
			Object.Parent = Parent;
		end
	end
end

local function UpdateDataTree(TreeFolder, Data)
	--| This iterates over the tables to ensure that the data exists
	for Index, Value in next, Data do -- you are unable to change a "category" into anything but a table, if this line errors you are tryin to change a category table into a string
		local Object = TreeFolder:FindFirstChild(Index);
		local Datatype = type(Value);
		if Datatype == "table" then
			if not Object then
				local Folder = Instance.new("Folder");
				Folder.Name = Index;
				UpdateDataTree(Folder, Value);
				Folder.Parent = TreeFolder;
			else
				UpdateDataTree(Object, Value);
			end
		else
			if not Object then
				local Object = Instance.new(Datatypes[Datatype]);
				Object.Value = Value;
				Object.Name = Index;
				Object.Parent = TreeFolder;
			else
				Object.Value = Value;
			end
		end
	end

	--| This removes any old data
	local Objects = TreeFolder:GetChildren()
	for i = 1, #Objects do
		local Object = Objects[i];
		if Data[Object.Name] == nil and (tonumber(Object.Name) and Data[tonumber(Object.Name)] == nil or tonumber(Object.Name) == nil) then
			Object:Destroy();
		end
	end
end

local function RecursiveUpdate(Tbl1, Tbl2)
	for Index, Value in next, Tbl2 do
		if Tbl1[Index] then
			if type(Value) == "table" and type(Tbl1[Index]) == "table" then
				RecursiveUpdate(Tbl1[Index], Value);
			else
				Tbl1[Index] = Value
			end
		else
			Tbl1[Index] = Value
		end
	end

	for Index, Value in next, Tbl1 do
		if Tbl2[Index] == nil then
			Tbl1[Index] = nil;
		end
	end
end

--|| Public Functions
function Datastore.new(UserId)
	local Player = Players:GetPlayerByUserId(UserId);
	local Data = {
		LastBackup = os.clock();
		Identifier = UserId;
		UpdateCallbacks = {};
		BeforeSaveCallbacks = {};
	};

	local PlayerData = Datastore:Load(UserId);

	if PlayerData then
		Data.Data = PlayerData;
		print("[DATASTORE LITE]: "..UserId.."'s PlayerData was loaded.");

		local Bindable, Fired = Instance.new("BindableEvent"), false;
		local Connection
		Connection = Player.AncestryChanged:Connect(function(_, Parent)
			if Player:IsDescendantOf(game) then return end; -- In case the player is reparented elsewhere
			if Parent == nil then
				Connection:Disconnect(); -- Shall only run once

				--| Run Callbacks Before Saving
				for i = 1, #Data.BeforeSaveCallbacks do
					local Callback = Data.BeforeSaveCallbacks[i];
					Callback(Data.Data);
				end

				Datastore:Save(UserId);
				Bindable:Fire();

				Cache[UserId] = nil;
			end
		end)

		--| Trigger AncestryChanged
		if RunService:IsStudio() and Configs.StudioSave or not Configs.StudioSave and not RunService:IsStudio() then
			game:BindToClose(function()
				if not Fired then
					coroutine.resume(coroutine.create(function()
						Player.Parent = nil;
					end))

					Bindable.Event:Wait(); -- Will yield until player data is saved.
					Bindable:Destroy();
				end;
			end)
		end

		--| Creates cache for OnUpdate connections
		for Index, Value in next, PlayerData do
			Data.UpdateCallbacks[Index] = {};
			if StarterData[Index] and StarterData[Index].CreateObjects then
				local Folder = Instance.new("Folder");
				Folder.Name = Index;
				Folder.Parent = Player;
				CreateDataTree(Folder, Value)
			end
		end
	else
		if Player then
			Player:Kick("We were unable to load your data, try rejoining or contact the owner if this issue persists.")
			print("[DATASTORE LIST]: Failed to load "..UserId.."'s Data, Player was kicked.")
			return nil;	
		end
	end

	local Object = setmetatable(Data, Datastore);
	Cache[UserId] = Object;

	for Index, Value in next, PlayerData do
		if StarterData[Index] and StarterData[Index].CreateObjects then
			local Folder = Player:FindFirstChild(Index);
			if Folder then
				Datastore:OnUpdate(UserId, Index, function(Old, New)
					UpdateDataTree(Folder, New);
				end)
			end
		end
	end

	return Cache[UserId];
end

function Datastore:OnUpdate(UserId, Index, Callback)
	local Bindable = Instance.new("BindableEvent");
	Bindable.Event:Connect(Callback);
	Cache[UserId].UpdateCallbacks[Index][#Cache[UserId].UpdateCallbacks[Index] + 1] = Bindable;
end

function Datastore:BeforeSave(UserId, Callback)
	if Cache[UserId] then
		Cache[UserId].BeforeSaveCallbacks[#Cache[UserId].BeforeSaveCallbacks + 1] = Callback;
	else
		warn("[DATASTORE LITE]: Could not find "..UserId.." in local cache")
	end
end

function Datastore:Backup(UserId)
	local UserId = UserId
	local PointerDatastore = GetOrderedDatastore(UserId);
	local BackupDatastore = GetDataStore(UserId.." Backup");
	local Data = Datastore:Get(UserId);
	local Timestamp = GetTimeStamp();
	local Time = os.time();

	local Success, Error = pcall(function()
		PointerDatastore:SetAsync(Timestamp, Time);
	end)

	if not Success then
		warn("[DATASTORE LITE]: Failed to set pointer data for; ", Error)
		return
	else
		Success, Error = pcall(function()
			BackupDatastore:SetAsync(Time, Data);
		end)

		if not Success then
			warn("[DATASTORE LITE]: Failed to set backup data for; ", Error)
		else
			print("[DATASTORE LITE]: Backup Data was saved for "..UserId)
		end
	end
end

function Datastore:Load(UserId)
	local PlayerDatastore = GetDataStore(UserId..Configs.DataVersion);
	local Success, Error, Attempts, Data = nil, nil, 0, nil;
	local InSession = false;
	while (Players:GetPlayerByUserId(UserId) 
		and Attempts < Configs.Attempts)
		and (not Success or InSession) do -- If you did not retrieve data properly or still in session

		Attempts += 1;
		Success, Error = pcall(function()
			PlayerDatastore:UpdateAsync("Data", function(OldData)
				--| Assume Player Is New
				if OldData == nil then

					InSession = false;
					local NewData = {};
					for Index, Value in next, StarterData do
						NewData[Index] = Copy(Value.Data);
					end		

					--| Set Session
					NewData.SessionData = GetSession();
					Data = NewData;
					return NewData;
				elseif OldData 
					and (OldData.SessionData == nil
						or os.time() - OldData.SessionData.Stamp >= 5) then

					InSession = false;
					--| Add New Data
					for Index, Value in next, StarterData do
						if OldData[Index] == nil then
							if Value.Data then
								OldData[Index] = Copy(Value.Data);
							else
								warn("[DATASTORE LITE]: When attempting to make a copy of "..Index.." data, the Data property was not located in ", Value)
							end
						end
					end

					--| Set Session
					OldData.SessionData = GetSession();
					Data = OldData;
					return OldData;
				elseif OldData and (OldData.SessionData and os.time() - OldData.SessionData.Stamp < 10) then
					InSession = true;
				end
			end)
		end)

		if (not Success or InSession) and Attempts < Configs.Attempts then
			wait(6);
		end
	end

	return Data;
end

function Datastore:Save(UserId)
	if RunService:IsStudio() and not Configs.StudioSave then warn("[DATASTORE LITE]: Attempted to save user data in ROBLOX STUDIO when StudioSave property is set to false") return end;
	local PlayerDatastore = GetDataStore(UserId..Configs.DataVersion);

	local Success, Error, Attempts = nil, nil, 0
	while not Success and Attempts < Configs.Attempts do
		Attempts += 1;
		Success, Error = pcall(function()
			PlayerDatastore:UpdateAsync("Data", function(OldData)
				if OldData.SessionData == nil or
					OldData.SessionData.JobId == game.JobId then
					return Cache[UserId].Data;
				end
			end)
		end)

		if Error and Attempts < Configs.Attempts then
			warn("[DATASTORE LITE]: Datastore failed to save "..Attempts.." times with; "..Error)
			wait(10);
		else
			print("[DATASTORE LITE]: "..UserId.."'s Data was saved successfully")
		end
	end

	if Error then
		warn("[DATASTORE LITE]: "..UserId.."'s Data failed to save.")
	end

	Datastore:Backup(UserId);
end

function Datastore:Set(Player, List, Value)
	if not Players:FindFirstChild(Player.Name) then return end;
	local Data = Datastore:Get(Player.UserId);
	local Index = List[1];
	local OldDirectory = Index and Copy(Cache[Player.UserId].Data[Index]);
	local Parent, IndexedValue, LastIndex = Data, Data, nil;
	for i = 1, #List do
		local Index = List[i];
		Parent = IndexedValue;
		IndexedValue = Parent[Index];
		LastIndex = Index;
	end

	if type(LastIndex) == "number" and Value == nil then
		table.remove(Parent, LastIndex);
	else		
		if type(Parent[LastIndex]) == "table" and type(Value) == "table" then
			RecursiveUpdate(Parent[LastIndex], Value);
		else
			Parent[LastIndex] = Value;
		end
	end

	if Index then
		local Bindables = Cache[Player.UserId].UpdateCallbacks[Index]
		for i = 1, #Bindables do
			local Bindable = Bindables[i];
			Bindable:Fire(OldDirectory, Cache[Player.UserId].Data[Index]);
		end
	end
end

function Datastore:Get(UserId)

	--[[
	
		[Description]: Yields until data is available, if player leaves while yielding will return nil
		
	]]
	if Cache[UserId] then
		return Cache[UserId].Data;
	else
		while Players:GetPlayerByUserId(UserId) and Cache[UserId] == nil do
			RunService.Stepped:Wait();
		end

		if Cache[UserId] then
			return Cache[UserId].Data;
		end
	end
end

function Datastore:GetAsync(UserId)
	local Data
	local Success, Error = pcall(function()
		Data = GetDataStore(UserId..Configs.DataVersion):GetAsync("Data");
	end)

	return Data;
end

--| Automatic Backups
coroutine.resume(coroutine.create(function()
	while true do
		local Players = Players:GetPlayers()
		for i = 1, #Players do
			local Player = Players[i];
			local Data = Cache[Player.UserId];

			if Data then
				if os.clock() - Data.LastBackup >= Configs.BackupInterval then
					Data.LastBackup = os.clock();
					coroutine.resume(coroutine.create(function()
						Datastore:Backup(Player.UserId);
					end))
				end
			end
		end
		wait(1)
	end
end))

return Datastore

If you wish to have a in-depth view of the model, the model is available right here:

Datastore Lite - Roblox

---

As of right now, the code will yield if a individual is “in session” however looking at the Datastore limits, I was wondering if it’d be safe to call GetAsync at a faster rate (in the scenario there is a session, I want to load the data as soon as possible). Micro-optimisations are appreciated but not what I’m looking for.

[DarthFS]

Alright, doesn’t look too shabby. Here’s a few tips:

• I recommend implementing proper-tail calls to save up space in the call stack.
  Ex. In your custom print and debug functions

-- Not Good
local function Print(...)
	if Configurations.Debug then
		print(script:GetFullName(), "\n", ...)
	end
end
-- Good
local function Print(...)
	if Configurations.Debug then
	    return print(script:GetFullName(), "\n", ...) -- Saves up callstack space.
 	end
end

There is a good read on the lua manual about this:
https://www.lua.org/pil/6.3.html

• I would replace this whole chunk of code to reduce cyclomatic complexity

-- Not Good
	local Success, Error = pcall(function()
		if Method == "GetAsync" then
			Data = Datastore:GetAsync(Name);
		elseif Method == "SetAsync" then
			Datastore:SetAsync(Name, Value);
		elseif Method == "RemoveAsync" then
			Datastore:RemoveAsync(Name);
		end
	end)
-- Good
local AsyncTable = {
   GetAsync = Datastore.GetAsync,
   RemoveAsync = Datastore.RemoveAsync,
   SetAsync = Datastore.SetAsync,
}
local function Async(Datastore, Method, Name, Value)
    local Data = nil
    local Success, Data = pcall(AsyncTable[Method], Datastore)

   	if not Success then
		Debug("Failed "..Method.." on "..Name, Error);
	else
		Print("Success "..Method.." on "..Name, Value);
	end
	
	return Success, Data; 
end

• I don’t really like your use of string.split. It seems heavily unneeded and overcomplicated in your :Write Method
• Don’t use numerical loops over ipairs, the way you are doing it. The generic loop is typically faster than the numeric. Numeric still has to do the calculation of [Paths[i]]. Here is my benchmark for visual proof.

local array = {}
for i = 1, 100 do
	array[#array + 1] = math.random(1, 10)
end

for i = 1, 100 do
	local numericTime = nil
	local ipairsTime = nil
	
	local startTime = os.clock()
	for i = 1, #array do
		local value = array[i]
	end
	numericTime = os.clock() - startTime
	startTime = os.clock()
	for index, value in ipairs(array) do
	end
	ipairsTime = os.clock() - startTime
	
	print(numericTime < ipairsTime and ('numeric is faster: %s'):format(numericTime) or ('ipairs is faster: %s'):format(ipairsTime))
end

• The only obscure edge cases where I would use numeric over ipairs is when I am iterating through an array backwards or by certain intervals, etc…
• Even when you would rather use numerical loops over the generic, please cache your values (Paths[i]). Indexing a key is not instantaneous.

for i = 1, #Paths do
     local value = Paths[i]
     -- Blah
end

• It’s less expensive to use signals as opposed to polling. Flooding the task scheduler every frame is not recommended. I’m not an expert on this so here is a more reliable post / source.
  Rainbow Items Script From client - #13 by Thedagz
• As I’ve stated before do cache your values:

-- Not Good
		if not CachedData[UserId] then return end 
		if not CachedData[UserId].Data then return end
		if CachedData[UserId].IsSaving then return end
-- Good
        local UserValue = CachedData[UserId] 
		if not UserValue or not UserValue.Data then return end 
		if UserValue.IsSaving then return end

• Usage of proper tail calls here is a good idea. Even if the value that is returned is not needed, it’s generally good practice:

function Datastore:Remove(UserId) --| This will wipe a player's current data
	local PlayerDatastore = GetPlayerDatastore(UserId);
	Datastore:Backup(UserId);
	return Async(PlayerDatastore, "RemoveAsync", "Data");
end

• In your :Load method, this should be placed at the top of your script to prevent the extra indexing each time you call that specific method:

local Unpacker = script.Parent.Assets.Unpacker

• Improper usage of :WaitForChild. The children of the player instance will always be loaded before run-time. Also PlayerGui should be cached in a variable at the top of your code. It’s good practice.

Unpacker.Parent = PlayerGui -- assuming you have a variable defined at the top of your script

• Don’t use next to iterate through an array. :GetPlayers method returns an array, so you should be using ipairs. You might be thinking the differences are negligible. But it’s good practice.

for _, Client in ipairs(players:GetPlayers()) do
end

• You can get rid of that scope you created when doing "if ReplicationMethod == “Private” since that scope is completely empty:

-- Good
	if Category and StarterData[Category] then
		ReplicationMethod = StarterData[Category].Replication;	
		if ReplicationMethod == "Global" then
			Updater:FireAllClients(Player.UserId.."."..Directory, Value, Number)
		elseif ReplicationMethod == "Local" then
			Updater:FireClient(Player, Player.UserId.."."..Directory, Value, Number)
		end
	end

• Naming a variable “Pointer” can be very misleading. If I’ve done my research correctly, all variables are assigned to a memory address / pointer.

These are the only tips I have. A lot of these “micro-optimizations” are good practice. A lot of other areas of your code where these tips need to be applied though. It’s not that bad of a script.

Happy Coding.

[angrybino]

I recommend implementing proper-tail calls to save up space in the call stack.
Ex. In your custom print and debug functions

Tail call implementation in Luau is disabled.

[DarthFS]

where is your source for this?

[angrybino]

local function foo()
wait()
return foo()
end

foo()

This code will cause a stack overflow after the function runs more than 16381 times even when it is using a tail call since it’s disabled in Luau. A tail call is just a way to prevent new stacks from being created when calling a function.

[binky007]

Could of just linked the luaU github Compatibility - Luau

[DarthFS]

My bad I assumed most of the lua manual was implemented into luau.