I found a good way to replicate the player data for the client but I'm wondering if its really good or no

Help and Feedback Code Review
,
#1

The title says it all, I want help and feedback on what should I improve on my data manager module but also I want feedback about the replication method I’m using in this code.

I have tested this replication method and it works fine, but is it really fine?

The replication method I’m using is simple:

  • Every time I call Increment or Set functions I call another function to replicate the new changes that have been made to the player’s data, I think its similar to how __newIndex works.
  • I send 3 arguments, the Player Object, the name of the data that is being Incremented or set, example: “Cash” and the newest value of “Cash” in sessionData[player.UserId].

Example:

 -- index is equivalent to "Cash" and the value is the newest value on the index "Cash" in sessionData[player.UserId] table.
self:OnUpdate(player, index, value) -- (player, "Cash", sessionData[index])

ModuleScript:

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local DataStoreService = game:GetService("DataStoreService")
local RunService = game:GetService("RunService")
local Players = game:GetService("Players")

local await = require(ReplicatedStorage.await) -- a custom wait

local DATA_STORE_NAME = "GLOBAL"
local DATA_STORE_SCOPE = "PlayerData"

local MAX_GET_ATTEMPTS = 6
local MAX_SAVE_ATTEMPTS = 8

local gameId = game.GameId
local jobId = game.JobId
local isStudio = RunService:IsStudio()

local DataStore = nil
if gameId ~= 0 then
	DataStore = DataStoreService:GetDataStore(DATA_STORE_NAME, DATA_STORE_SCOPE)
end

local DataListener = ReplicatedStorage.Remotes.DataStoreRequests.DataListener

local sessionData = {}
local defaultData = {
	Cash = 0
}

local function loadPlayerData(player)
	local id = tostring(player.UserId)
	local attempts = 0
	local success, result = nil, nil

	sessionData[id] = defaultData

	while not success and attempts < MAX_GET_ATTEMPTS do
		success, result = pcall(DataStore.GetAsync, DataStore, id)
		if not success then
			attempts = attempts + 1
			warn(result)
			await(1)
		end
	end

	if success then
		if result and (type(result) == "table") then
			sessionData[id] = result
		end

		local BoolValue = Instance.new("BoolValue")
		BoolValue.Name = "finished " .. player.Name
		BoolValue.Value = true
		BoolVlaue.Parent = game.ReplicatedStorage.PlayerData
	else
		warn(result)
	end
end

local function savePlayerData(player)
	if isStudio then
		return
	end

	local id = tostring(player.UserId)
	local attempts = 0
	local success, result = nil, nil

	while not success and attempts < MAX_SAVE_ATTEMPTS do
		success, result = pcall(DataStore.UpdateAsync, DataStore, id, function(data)
			if data == nil then
				return sessionData[id]
			elseif data ~= nil and data.SessionJobId == nil or data.SessionJobId == jobId then
				data.SessionJobId = jobId
				if data ~= sessionData[id] then
					return sessionData[id]
				end
				return nil
			end
			return nil
		end)
		if not success then
			attempts = attempts + 1
			warn(result)
			await(6)
		end
	end

	if success then
		warn(("[DataStore]: %s's data has been saved with %s attempts."):format(player.Name, attempts))
	else
		warn(result)
	end

	sessionData[id] = nil
end

for _, player in ipairs(Players:GetPlayers()) do
	coroutine.wrap(loadPlayerData)(player)
end

Players.PlayerAdded:Connect(loadPlayerData)
Players.PlayerRemoving:Connect(savePlayerData)

game:BindToClose(function()
	if not isStudio then
		for _, player in ipairs(Players:GetPlayers()) do
			coroutine.wrap(savePlayerData)(player)
		end
	end
end)

local DataManager = {}

DataManager.__index = DataManager

function DataManager:GetData(player)
	assert(player:IsA("Player") == true)

	local id = tostring(player.UserId)

	if sessionData[id] then
		return sessionData[id]
	end
	return nil
end

function DataManager:Get(player, index)
	local data = self:GetData(player)

	if data and data[index] then
		return data[index]
	end
	return nil
end

function DataManager:Set(player, index, value)
	local data = self:GetData(player)

	if data and data[index] and (type(data[index]) == type(value)) then
		data[index] = value
	end
	self:OnUpdate(player, index, data[index]) -- ahem
end

function DataManager:Increment(player, index, value)
	assert(type(value) == "number", "Argument 3 must be a number.")
	
	local data = self:GetData(player)
	
	if data and data[index] then
		data[index] = data[index] + value
	end
	self:OnUpdate(player, index, data[index]) -- ahem
end

--[[
	function that will handle the data replication for the client.
	how it works?;
	:OnUpdate(player, index, value) is called every time set & increment functions are called.
	it works like a __newIndex metamethod, i guess.
--]]
function DataManager:OnUpdate(player, ...)
	DataListener:FireClient(player, ...)
end

return DataManager

Server:

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")

local DataManager = require(game:GetService("ServerStorage").DataManager)

local BindableEvent = Instance.new("BindableEvent")

-- this is how I wait my data to load, using ChildAdded:Wait() and I think its the right way to do it.
-- learn more about it: https://devforum.roblox.com/t/avoiding-wait-and-why/244015
local function onChildAdded(player, child)
	if child.Name == "finished " .. player.Name then
		BindableEvent:Fire()
		child:Destroy()
	end
end

Players.PlayerAdded:Connect(function(player)
	ReplicatedStorage.PlayerData.ChildAdded:Connect(function(child)
		childAdded(player, child)
	end)
	BindableEvent.Event:Wait()

	DataManager:Increment(player, "Cash", 50)
end)

Client:

local ReplicatedStorage = game:GetService("ReplicatedStorage")

local Remotes = ReplicatedStorage.Remotes
local DataStoreRequests = Remotes.DataStoreRequests

DataStoreRequests.DataListener.OnClientEvent:Connect(function(index, value)
	print(index, value) -- Cash, 50
end)
#2

Your way of replicating data to the client looks fine, if you aren’t going to anything with index and value, just send the updated data fully (assuming it’s not cyclic).

local player_Data 

DataStoreRequests.DataListener.OnClientEvent:Connect(function(data)
	player_Data  = data 
end)

Reviewing your module script:

  • Unnecessary function and expensive function calls

Indexing a table numerically, Luau automatically converts that number into a string. Therefore local id = tostring(player.UserId) is redundant, instead:

local id = player.UserId
  • Nit picks
    Also, there is no point of allocating memory on the stack, just do sessionData[player.UserId] for indexing.

Luau also introduced the += compound.

if not success then
			attempts += 1 --> attempts = attempts + 1
			warn(result)
			await(1)
		end

Don’t allocate unnecessary variables on the stack.

DataManager.__index = {}
  • Truthy values

If the data you’re saving is a table, it will always be a table. There is no point at which it wouldn’t be a table unless the API has returned the incorrect data which it wouldn’t.

if result and (type(result) == "table") then
			sessionData[id] = result
		end
1 Like
#3
#4