Is this datastore secure?

Help and Feedback Code Review
#1

I made a datastore script for my game but obviously its very important to me to avoid all dataloss if possible. I’m aware that :SetAsync() is a good way to overwrite data, but I’m not sure how else to go about it.

Here’s my script for everyone to review (sorry I don’t use comments, I’m aware its not good.)

local dataService = game:GetService("DataStoreService")
local players = game:GetService("Players")
local Data = dataService:GetDataStore("Stats")

local TimerLeaderboard = dataService:GetOrderedDataStore("TimerLeaderboard")
local MergesLeaderboard = dataService:GetOrderedDataStore("MergesLeaderboard")
local CoinsLeaderboard = dataService:GetOrderedDataStore("CoinsLeaderboard")

local sessionData = {}
local plrsIngame = {}

local function updateLeaderboard()
	for i,plr in plrsIngame do
		local success, errorMsg = pcall(function()
			if sessionData[plr.UserId] then
				TimerLeaderboard:SetAsync(plr.UserId, sessionData[plr.UserId].Timer)
				CoinsLeaderboard:SetAsync(plr.UserId, sessionData[plr.UserId].Coins)
				MergesLeaderboard:SetAsync(plr.UserId, sessionData[plr.UserId].Merges)
			end	
		end)
		if not success then
			warn("Failed to update leaderboard:", errorMsg)
		else
			print("Done updating global leaderboard for server")
		end
	end
end

local function joined(plr)
	table.insert(plrsIngame, plr)
	local Folder = Instance.new("Folder")
	Folder.Name = "Stats"
	Folder.Parent = plr
	local Merges = Instance.new("NumberValue")
	Merges.Name = "Merges"
	Merges.Parent = Folder
	local Timer = Instance.new("NumberValue")
	Timer.Name = "Timer"
	Timer.Parent = Folder
	local Coins = Instance.new("NumberValue")
	Coins.Name = "Coins"
	Coins.Parent = Folder
	
	local leaderstats = Instance.new("Folder")
	leaderstats.Name = "leaderstats"
	leaderstats.Parent = plr
	local Points = Instance.new("NumberValue")
	Points.Name = "Points"
	Points.Parent = leaderstats
	local Wins = Instance.new("NumberValue")
	Wins.Name = "Wins"
	Wins.Parent = leaderstats

	local success,playerData = pcall(function()
		return Data:GetAsync(plr.UserId)
	end)
	if success then
		if not playerData then
			playerData = {
				["Merges"] = 0,
				["Timer"] = 0,
				["Coins"] = 0,
				["Wins"] = 0
			}
		end
		sessionData[plr.UserId] = playerData
	else
		plr:Kick("Could not load Data, please rejoin.")
	end

	Merges.Value = sessionData[plr.UserId].Merges
	Timer.Value = sessionData[plr.UserId].Timer
	Coins.Value = sessionData[plr.UserId].Coins
	Wins.Value = sessionData[plr.UserId].Wins

	Merges:GetPropertyChangedSignal("Value"):Connect(function()
		sessionData[plr.UserId].Merges = Merges.Value
	end)
	Timer:GetPropertyChangedSignal("Value"):Connect(function()
		sessionData[plr.UserId].Timer = Timer.Value
	end)
	Coins:GetPropertyChangedSignal("Value"):Connect(function()
		sessionData[plr.UserId].Coins = Coins.Value
	end)
	Wins:GetPropertyChangedSignal("Value"):Connect(function()
		sessionData[plr.UserId].Wins = Wins.Value
	end)
end

local function leaving(plr)	
	table.remove(plrsIngame, table.find(plrsIngame, plr))
	if sessionData[plr.UserId] then
		local success, errorMsg = pcall(function()
			Data:SetAsync(plr.UserId, sessionData[plr.UserId])
			TimerLeaderboard:SetAsync(plr.UserId, sessionData[plr.UserId].Timer)
			CoinsLeaderboard:SetAsync(plr.UserId, sessionData[plr.UserId].Coins)
			MergesLeaderboard:SetAsync(plr.UserId, sessionData[plr.UserId].Merges)
			sessionData[plr.UserId] = nil
		end)
		if not success then
			warn(errorMsg)
		end
	end
end

players.PlayerAdded:Connect(joined)
players.PlayerRemoving:Connect(leaving)

task.spawn(function()
	while task.wait(60) do
		updateLeaderboard()
	end
end)
#2

Looks Good in my opinion

But also Use BindToClose in case of Server suddenly shutdown unexpectedly

3 Likes
#3

Very good idea, thank you! I did not even consider this.

Just so we’re sure (I’ve never used bindtoclose before allthough I do know of its existance)
This would work fine, right?

game:BindToClose(function()
	for i,v in game.Players:GetChildren() do
		if v:IsA("Player") then -- I have some values in game.Players so I'll do a safecheck
			leaving(v)
		end
	end
end)
#4

Off topic here, but a way to make your code base smaller is to have a table if all the Number Values (bools, strings & etc…). But in this case, you have a stats folder & a leaderstats folder so would split them into two.

heres an example

local leaderstats_NumberValues = {"Points","Wins"}

local function joined(plr)
	local leaderstats = Instance.new("Folder",plr)
	leaderstats.Name = "leaderstats"
	
	for i,v in pairs(leaderstats_NumberValues) do
		local value = Instance.new("NumberValue",leaderstats)
		value.Name = v
	end
end

In this little example you can add values in the future & if you going to add different functionality to this script it’ll be easier to read (I don’t know what else you might add but you get my point, at least I hope). Anyways more on topic.

In this you’re doing everything right but there is an improvement, instead of kicking immediately why not you try 5 times of getting the data and if it fails then kick them

local try_max_ammount = 5

local function joined(plr)
	-- After intialize code
	
	local success,err
	local try_ammount = 0
	
	repeat 
		success,err = pcall(function()
			-- yada yada
		end)
		try_ammount += 1
		if not success then
			warn(err)
			task.wait(1)
		end
		if success then
			break
		end
	until try_ammount == try_max_ammount or success == true
	
	if success == true then
		-- ect...
	else
		plr:Kick("Could not load Data, please rejoin.")
	end
end

Anyways these are examples you can try to integrate them.

#5

While a simple pcall is fine for most saving cases you can implement a retry logic and attempt to save the data at least 3 times.

The current use of warn() is fine, you could add more context (like the userid) to help with debugging.