How can I improve the structure of this data saving/loading code?

Help and Feedback Code Review
1

Hey there,

As the title says, how can I improve this? What I am looking for is:

  • Optimization
  • Improve readability

Thanks. :heart:

local dss = game:GetService("DataStoreService")
local levelDS = dss:GetDataStore("MainData")
local MAX_RETRIES, RETRY_DELAY = 10, 1

local defaults = {
	Sticks = 0,
	Iron = 0,
	Gold = 0,
	Coal = 0,
	Wood = 0,
	Stone = 0,
	Relic1 = false,
	Relic2 = false,
	ClanName = "",
	ClanID = "",
	ClanRank = "",
	Guild = "",
	Tooltips = false,
	Autojump = false,
	Music = 0,
	JetskiUnlocked = false,
	SpeedboatUnlocked = false,
	YachtUnlocked = false
}

local function safeGetAsync(key)
	for i = 1, MAX_RETRIES do
		local success, data = pcall(function()
			return levelDS:GetAsync(key)
		end)
		if success then
			return data or {}
		end
		task.wait(RETRY_DELAY)
	end
	return nil
end

local function safeSetAsync(key, data)
	for i = 1, MAX_RETRIES do
		local success = pcall(function()
			levelDS:SetAsync(key, data)
		end)
		if success then
			return true
		end
		task.wait(RETRY_DELAY)
	end
	return false
end

local function saveData(player)
	local key = "Player_" .. tostring(player.UserId)
	local data = {}
	for attr, default in pairs(defaults) do
		data[attr] = player:GetAttribute(attr)
	end
	if not safeSetAsync(key, data) then
		warn("Critical failure: Could not save data for player " .. player.UserId)
	end
end

local function loadData(player)
	local key = "Player_" .. tostring(player.UserId)
	local data = safeGetAsync(key) or {}
	for attr, default in pairs(defaults) do
		player:SetAttribute(attr, (data[attr] ~= nil) and data[attr] or default)
	end
end

game.Players.PlayerAdded:Connect(function(player)
	loadData(player)
	player.CharacterAdded:Connect(function()
		task.wait(1)
		loadData(player)
	end)
end)

game.Players.PlayerRemoving:Connect(saveData)

game:BindToClose(function()
	for _, player in pairs(game.Players:GetPlayers()) do
		saveData(player)
	end
end)
2

separate default constants into a new module script (Constants/DatastoreConfig.luau or something):

local defaults = {
	Sticks = 0,
	Iron = 0,
	Gold = 0,
	Coal = 0,
	Wood = 0,
	Stone = 0,
	Relic1 = false,
	Relic2 = false,
	ClanName = "",
	ClanID = "",
	ClanRank = "",
	Guild = "",
	Tooltips = false,
	Autojump = false,
	Music = 0,
	JetskiUnlocked = false,
	SpeedboatUnlocked = false,
	YachtUnlocked = false
}

return defaults

always define our services at the top so that other viewers know what services are used here:

-- the names should be pascal case
local DataStoreService = game:GetService("DataStoreService")
local Players = game:GetService("Players")

require the config:

local DataStoreService = game:GetService("DataStoreService")
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
-- In this case, I usually store my config in ReplicatedStorage.

local DefaultConfig = require(ReplicatedStorage.Constants.DefaultConfig)

apply config to your loading process:

local function loadData(player: Player)
	local key = `Player_{player and player.UserId:tostring()}`
	local data = safeGetAsync(key) or {}
	-- Because you defaulted "data" to {}, it's okay to avoid "== nil" conditions.
	for attributeName, defaultValue in DefaultConfig do
		local value = data[attributeName] or defaultValue
		player:SetAttribute(attributeName, value)
	end
end

as for your retries, it’s better to have a queue because when your fetching fails, the thread continues until the retry is completed. so for every player that joins your game, lets assume 20 players join your game at the same time and all of them fail: there will be 20 existing for-loop threads requesting data from DataStoreService at the same time. therefore, you’ll still be bottlenecked for 1 request per X minutes / seconds. when you replace this retry process with a queue, you can reduce the thread count to 1. that way, you can use the other 19 threads for other tasks.

a good practice for any requests to an API is to try to stay below the request threshold. when you start to use an API that requests data from external endpoints like the website, you will more than likely face these types of limitations. so as a developer, it’s important to design a system that can still perform the maximum efficiency, but also reduce efficiency to the quota until the request threshold is marginally far from your current. i hope that makes sense.

1 Like
3

Correct me if I’m wrong but it seems like you have nothing that would prevent saving the default data when the correct data of a player hasn’t been successfully loaded. So if a player had data stored on earlier sessions but that data failed to load on one session, wouldn’t the player lose their data?

4

Little bit of nitpicking, but when saying naming cases in comments, documentation, etc, it’s good to put the name of the case in the case itself. For instance,

camelCase
PascalCase
snake_case
LOUD_SNAKE_CASE
kebab-case

So, the comment would be

-- the names should be PascalCase