This is my first time making a code for a leaderboard in my game. I didn’t really do much research, only about OrderedDataStores. For that reason, I really want to know if there’s anything I could improve, specially about the data saving part of the code. I’m afraid if this amount of :SetAsync()s could be a bad practice.
I’ll emphasize that my game doesn’t have many players, rarely there are enough players to have more than 1 server (the size of one server is 25). Even though, I still want to have the best code possible for this, specially because who knows if the game will get bigger.
local dataStoreService = game:GetService("DataStoreService")
local players = game:GetService("Players")
local boardDataStore = dataStoreService:GetOrderedDataStore("AllTimeKills")
local leaderboardModel = script.Parent
local boardSize = 25
local updateTime = 60 * 5 -- 5 minutes
local function SetPlayerKills(player) -- save the kills in the new global data store
local kills = player:WaitForChild("leaderstats"):WaitForChild("Total Kills").Value
boardDataStore:SetAsync(player.UserId, kills)
end
for _, player in players:GetPlayers() do -- set in case a player joined before the PlayerAdded signal is connected
SetPlayerKills(player)
end
players.PlayerAdded:Connect(SetPlayerKills)
players.PlayerRemoving:Connect(SetPlayerKills)
-- saves their kills once they join and leave
local function LoadLeaderboard()
local orderedDataPages: DataStorePages = boardDataStore:GetSortedAsync(false, boardSize)
local pagesInfo: {{key: string, value: any}} = orderedDataPages:GetCurrentPage()
-- getting the pages in the function in case a player gets more kills than the 25th place for example, which would remove them from the page
for rank, data in pagesInfo do
local userId = data.key
local kills = data.value
local chosenSlot = leaderboardModel.Leaderboard.ScrollingFrame[tostring(rank)]
chosenSlot.PlayerName.Text = players:GetNameFromUserIdAsync(userId)
chosenSlot.Kills.Text = tostring(kills)
end
end
while true do
-- saves the new kills of the players each 5 minutes, also updating the leaderboard between multiple servers
for _, player in players:GetPlayers() do
SetPlayerKills(player)
end
LoadLeaderboard()
task.wait(updateTime)
end
I would personally individually check for leaderstats and total kills individually with FindFirstChild because it’ll avoid errors. Also, the Value might be nil or 0 when a player first joins so their total kills will be set to 0.
You should error handle this using pcall’s and maybe add a retry system if it fails to save
You should remove this because when a player leaves and then rejoins they’ll have the same data so this does nothing. Player removing is enough
Reduce calls to SetAsync() (It is rate limited)
Use .Changed event to detect when there is a change before using SetAsync()
local function SetPlayerKills(player)
local kills = player:WaitForChild("leaderstats"):WaitForChild("Total Kills")
local lastSavedValue = kills.Value
kills.Changed:Connect(function(newKills)
if newKills ~= lastSavedValue then
-- Save only if the value has changed
lastSavedValue = newKills
boardDataStore:SetAsync(player.UserId, newKills)
end
end)
boardDataStore:SetAsync(player.UserId, kills.Value)
end
It’s important to implement error handling around any DataStore
local function SetPlayerKills(player)
local kills = player:WaitForChild("leaderstats"):WaitForChild("Total Kills").Value
local success, errorMessage = pcall(function()
boardDataStore:SetAsync(player.UserId, kills)
end)
if not success then
warn("Failed to save data for player " .. player.Name .. ": " .. errorMessage)
end
end
local function LoadLeaderboard()
local success, orderedDataPages = pcall(function()
return boardDataStore:GetSortedAsync(false, boardSize)
end)
if success then
local pagesInfo = orderedDataPages:GetCurrentPage()
for rank, data in ipairs(pagesInfo) do
local userId = data.key
local kills = data.value
local chosenSlot = leaderboardModel.Leaderboard.ScrollingFrame[tostring(rank)]
chosenSlot.PlayerName.Text = players:GetNameFromUserIdAsync(userId)
chosenSlot.Kills.Text = tostring(kills)
end
else
warn("Failed to load leaderboard data: " .. orderedDataPages)
end
end
Try batch updating so you don’t overload
Make use of dictionary
local function SaveAllPlayerKills()
local playerData = {}
for _, player in players:GetPlayers() do
local kills = player:FindFirstChild("leaderstats") and player.leaderstats:FindFirstChild("Total Kills")
if kills then
playerData[player.UserId] = kills.Value
end
end
for userId, kills in pairs(playerData) do
local success, errorMessage = pcall(function()
boardDataStore:SetAsync(userId, kills)
end)
if not success then
warn("Failed to save data for player with UserId " .. tostring(userId) .. ": " .. errorMessage)
end
end
end
Funny enough, your code below will increase the amount SetAsync is called. If a player constantly kills others they’ll eventually get rate limited because you’re saving everytime the amount of total kille increases
What do you mean “overload”? Having one loop that saves the player data is better that 2 loops in terms of efficiency
That can be true if the game has frequent kills, however if you do a batch system you won’t have that problem which I suggest doing. You can help reduce calls for when no killing is occurring if you setup a batch system.
The way you have it setup it’ll attempt to save data for all the players at once which might have a problem with overload causing calls to drop. Doing a batch system can help reduce the amount called at a single time. What I setup for you is meant as just a start that has error checks. You may want to be adding some delays to it for example this part:
for userId, kills in pairs(playerData) do
local success, errorMessage = pcall(function()
boardDataStore:SetAsync(userId, kills)
end)
if not success then
warn("Failed to save data for player with UserId " .. tostring(userId) .. ": " .. errorMessage)
end
task.wait(0.25) --Small delay between each data save
end
Saving the UserId to the batch dictionary means even if the player leaves you still got the info you need to save their data.
Hope this helps
Edit: Just noticed you aren’t the OP so when I say “you” I mean the OP
If it’s a typical server of 12 and you’re not close to the limit making calls that fast is actually fine. Also, the delay on the bottom might be problematic if the server shuts down
I don’t know, I generally code things to handle situations that likely never would come up just so it doesn’t ever have an issue. Batch-system is a good idea for something that is requiring constant DataStore saves. OP asked for improvement suggestions so that’s one of the ideas I had.
Hey @sonic_848 and @SLPM ! Thank you both for all the suggestions!
To reduce the calls of :SetAsync(), I basically saved the last saved kill values in a table so I could check if the kills are still the same whenever they’re about to get saved. I also removed function on the PlayerAdded event and I am using pcall() to save the data.
local dataStoreService = game:GetService("DataStoreService")
local players = game:GetService("Players")
local replicatedStorage = game:GetService("ReplicatedStorage")
local numberFormatter = require(replicatedStorage.Modules.NumberFormatter)
local boardDataStore = dataStoreService:GetOrderedDataStore("AllTimeKills")
local leaderboardModel = script.Parent
local boardSize = 25
local updateTime = 60 * 5
local lastSavedValues: {[number]: number} = {}
local function SetPlayerKills(player)
local kills: number = player:WaitForChild("leaderstats"):WaitForChild("Total Kills").Value
if lastSavedValues[player.UserId] ~= kills then
local success, result = pcall(function()
boardDataStore:SetAsync(player.UserId, kills)
end)
if not success then
warn("Failed to set " .. player.Name .. "'s kills to leaderboard: " .. result)
else
lastSavedValues[player.UserId] = kills
end
end
end
players.PlayerRemoving:Connect(function(player)
if not replicatedStorage.ShuttingDown.Value then
SetPlayerKills(player)
end
end)
game:BindToClose(function()
for _, player in players:GetPlayers() do
SetPlayerKills(player)
end
end)
local function LoadLeaderboard()
local orderedDataPages: DataStorePages = boardDataStore:GetSortedAsync(false, boardSize)
local pagesInfo: {{key: string, value: any}} = orderedDataPages:GetCurrentPage()
for rank, data in pagesInfo do
local userId = data.key
local kills = data.value
local chosenSlot = leaderboardModel.Leaderboard.ScrollingFrame[tostring(rank)]
chosenSlot.PlayerName.Text = players:GetNameFromUserIdAsync(userId)
chosenSlot.Kills.Text = numberFormatter:FormatNumber(kills)
end
end
while true do
for _, player in players:GetPlayers() do
SetPlayerKills(player)
end
LoadLeaderboard()
task.wait(updateTime)
end
