local ServerStorage = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game.Players
local MarketPlaceService = game:GetService("MarketplaceService")
local DataStoreService = game:GetService("DataStoreService")
local StatsDS = DataStoreService:GetGlobalDataStore("StatsDatastore")
local InventoryDS = DataStoreService:GetGlobalDataStore("InventoryDataStore")
local MainDS = DataStoreService:GetDataStore("MainDataStore")
local PlayerCount = #game.Players:GetChildren()
local HttpService = game:GetService("HttpService")
Players.PlayerAdded:Connect(function(player)
local Stats = Instance.new("Folder")
Stats.Name = "PlayerStats"
local Inventory = Instance.new("Folder")
Inventory.Name = "Inventory"
local Coins = Instance.new("NumberValue")
Coins.Name = "Coins"
local Hotbar = Instance.new("StringValue")
Hotbar.Name = "Hotbar"
Stats.Parent = player
Inventory.Parent = player
Coins.Parent = Stats
Hotbar.Parent = Inventory
local main = nil
local success,errormsg = pcall(function()
main = MainDS:GetAsync(player.UserId)
end)
if success and main ~= nil then
local p = HttpService:JSONDecode(main).playerstats
local i = HttpService:JSONDecode(main).inventory
for i,value in pairs(player.PlayerStats:GetChildren()) do
if value:IsA("ValueBase") then
for i=1,#p do
if p[i][1] == value.Name then
value.Value = tonumber(p[i][2])
end
end
end
end
end
end)
Players.PlayerRemoving:Connect(function(player)
wait()
local datasave = {["playerstats"]={},["inventory"]={}}
for i,value in pairs(player.PlayerStats:GetChildren()) do
if value:IsA("ValueBase") then
local ps = datasave.playerstats
table.insert(ps,#ps+1,{value.Name,value.Value})
end
end
for i,block in pairs(player.Inventory:GetChildren()) do
if block:IsA("NumberValue") then
local I = datasave.inventory
table.insert(I,#I+1,{block.name,block.Value})
end
end
local success,m = pcall(function()
MainDS:SetAsync(player.UserId,HttpService:JSONEncode(datasave))
end)
if m then
warn(m)
else
print(HttpService:JSONEncode(datasave))
print(player.Name.."'s Data Was Saved")
end
end)
As with every other case of a DataStore code review, use UpdateAsync instead of SetAsync if you don’t want the most annoying possible issues with data loss.
I’m not sure how exactly you’re using custom compression when encoding the data to JSON after doing your logic.
When I think of custom compression I “super-compress” the data with absolutely no redundant information in the table. Redundant data being any data that’s in the table that could be declared in a script instead.
If you want me to explain the logic further please notify me. Basically, instead of having {name=“john doe”}, I am doing “john doe” and that’s it. It’s just a string, then I can separate the next values with a comma. Then, you can use a compression library like this one: Text compression
I actually use my own module of a custom JSON like data format that removes the redundant parts of JSON (the size is only decreased by 5%-10% , but that’s still something), then run it through a simple LZW function. That would also work for JSON.
There are a couple of issues I’ve noticed with your code. Note that I’m going in with the assumption that you’ve laid everything bare in this script and left nothing out.
If you do have other processes running but didn’t include them, it’d be helpful to cut out the variables linked to those other processes as well so it doesn’t look as though you have needless declarations. For example, MarketplaceService is declared but never used as far as code reviewers are concerned.
I. Excess of variables.
The top of your script, which holds your variable declarations, very simply put, is ugly to look at. In the cases where you don’t use any of those variables or need them declared immediately, get rid of them. Here’s a list of what I’m looking at:
ServerStorage: Not used anywhere in your code.
ReplicatedStorage: Ditto.
MarketplaceService: Ditto.
HttpService: Use of JSON is unnecessary. Roblox does this under the hood and all you achieve by encoding is inflating your DataStore character count. Either use a custom format or save raw. If you do this for the sake of compression, look into other compression strategies.
Also worth noting that there’s maximum of 200 local upvalues. You probably won’t ever hit that limit but just keep that in the back of your mind.
II. Inconsistency, redundancy and poor organisation of variables.
One very clear example is that you index the Players service via dot syntax and go on later to write the full service path again but to use GetChildren (which is canonically incorrect, you should be using GetPlayers). If you set up a variable, then use the path.
As far as poor organisation of variables goes, you just have a mass of declarations at the top. Try putting them into a clearer format and separating them with new lines. I personally put service declarations at the top, followed by constants and variables.
local Players = game:GetService("Players")
local DataStoreService = game:GetService("DataStoreService")
-- Including "Datastore" in a DataStore namespace is unnecessary!
-- If you're working with a DS namespace, the fact that it's a DS is implied.
local MainStore = DataStoreService:GetDataStore("Main") -- See III.
local StatsStore = DataStoreService:GetDataStore("Stats") -- See IV.
local InventoryStore = DataStoreService:GetDataStore("Inventory") -- Ditto.
III. Potential excessive usage of DataStores.
In the code sample you’ve provided, you have three DataStores set up per player. This means that per player, you use three requests of any budget for any task. You need to keep DataStore limitations in mind. Even worse, if you decide to change to DataStore2 in the future, you increase your request amount to n*2 requests, ignoring combined stores.
You don’t need three DataStores! I don’t even see you using the other two! When working with DataStores, you should always try aiming to consolidate your stores as much as possible with the use of dictionaries. Create partitions in a larger table and save there.
You’re using the wrong function in this case. GetGlobalDataStore returns the default DataStore and should only be used if you need game-wide configuration access. GetGlobalDataStore does not accept a name parameter, so the string you pass to it is thrown away.
Simply put: you may experience overwrites or data loss by using GetGlobalDataStore and you shouldn’t be using it for player data in the first place. You must use GetDataStore for named DataStores and for partitioning.
V. Parenting children in the wrong order.
In any typical scenario, you should always set parents from the bottom up rather than the top down. This almost guarantees that the descendants will be available when the ancestor replicates. It’s always good practice to have the descendants available when the ancestors are!
Hotbar.Parent = Inventory
Coins.Parent = Stats
Inventory.Parent = player
Stats.Parent = player
On that note though, a small practice related thing: Coins probably belongs in inventory and you should aim to include all these stats either in some kind of single folder hierarchy, as such:
ex1
Player
Data
Inventory
Hotbar
Stats
Coins
Player
Data
Hotbar
Coins
VI. Pcall wrapping.
Okay, I really need to write that pcall tutorial.
You should try to stay away from using upvalues to get pcall values. Pcall is a callback, therefore you can return the results of a pcall or directly wrap the function call. I highly suggest doing the latter. Here are the examples of returning and direct wrapping:
local success, result = pcall(function ()
return MainDS:GetAsync(player.UserId)
end)
local success, result = pcall(MainDS.GetAsync, MainDS, player.UserId)
VII. Iterating over an array.
If you’re iterating over an array, the keys of it are guaranteed to be in numerical order and not arbitrary. In this case, please use ipairs and not pairs. ipairs runs faster than pairs, especially with LuaU and ipairs was designed specifically with arrays in mind while pairs for dictionaries.
This applies for all instances of your use of pairs while iterating through arrays (GetChildren generates and returns an array). Please go back and correct all of those!
VIII. Yielding at the start of PlayerRemoving.
Simply put: don’t. You don’t need to. Remove the wait.
Here’s a loosely (or possibly directly) related thread that you’d should read. The short of it is that wait has no real place in production code.