Hey Guys, I trying to move my game into Beta and I have just one more hurdle. Making my pet saving data bulletproof.
Ok, i get it, it’s ROBLOX, but when a player says ‘I’ve lost all my pets’ I just want to cringe and hide in the corner.
Anyway, today I’m manning up and started to look through the code again, but honestly it looks fine to this noob.
So please could you run your eyes over the code to make sure I’m not missing anything. (more specifically I cringe that the SetAsync functions are overwriting existing data due to some random roblox issue)
local Players = game:GetService("Players")
local DSService = game:GetService("DataStoreService")
local data = DSService:GetDataStore("TablePets003")
Players.PlayerAdded:Connect(function(player)
Instance.new("Folder",player).Name = "Pets"
Instance.new("Folder",player).Name = "PetsEquiped"
local key = player.UserId
local Pets = player:WaitForChild("Pets")
for i,v in pairs(game.ReplicatedStorage:WaitForChild("Pets"):GetChildren()) do
Instance.new("IntValue",Pets).Name = tostring(v)
end
wait(1)
local savedLevel = data:GetAsync(key)
print("before dump")
print(savedLevel)
print("after dump")
if savedLevel ~= nil then
for i, data in pairs(savedLevel) do
local pet = Pets:FindFirstChild(data[1])
if pet then
pet.Value = data[2]
else
local instance = Instance.new(data[3])
if instance then
instance.Name = data[1]
instance.Value = data[2]
instance.Parent = Pets
end
end
print(data)
end
else
data:SetAsync(key, {})
end
print(data)
end)
Players.PlayerRemoving:Connect(function(player)
local key = player.UserId
local savedLevel = data:GetAsync(key)
local Pets = player:FindFirstChild("Pets")
if not Pets then return end
Pets = Pets:GetChildren()
local objData = {}
for i, obj in pairs(Pets) do
table.insert(objData, {obj.Name, obj.Value, obj.ClassName})
end
if savedLevel == nil then
data:SetAsync(key, objData)
else
data:UpdateAsync(key, function(oldValue)
oldValue = objData
return objData
end)
end
end)
game:BindToClose(function()
for i, player in pairs(Players:GetPlayers()) do
local key = player.UserId
local savedLevel = data:GetAsync(key)
local pets = player:FindFirstChild("Pets")
if not pets then return end
pets = pets:GetChildren()
local objData = {}
for i, obj in pairs(pets) do
table.insert(objData, {obj.Name, obj.Value, obj.ClassName})
end
if savedLevel == nil then
data:SetAsync(key, objData)
else
data:UpdateAsync(key, function(oldValue)
oldValue = objData
return objData
end)
end
end
wait(3)
end)
PS: I was hoping to convert to Datastore2 but it’s way too complicated to convert for my experience level.
You cannot assume that the old data was saved and destroyed when PlayerAdded runs. The player may be rejoining the same server.
Calls to DataStore getters and setters can fail. You need to call them in protected mode with pcall and check if they succeeded. You need to check if the data load call succeeded when you go to save player data.
Don’t assume that player data has completed loading when the player leaves.
You don’t need to do anything else when the game shuts down other than wait until the normal PlayerRemoving listeners complete.
You should be storing the player data in a table, not making another request to the DataStore when you want to save data.
Don’t store pets by themselves. All player data belongs in a single table if it can fit, with possibly some exceptions for arbitrary user-generated content data and things like that. When the player’s data is loaded, compare the version of that data (meaning the version of the template / schema the data was made from). If loaded data is newer than the current server, merge it in authoritatively, where if a key does not exist in the current server data, it is written to. If it is not newer, ignore these keys. This allows you to easily add and remove data, and support old servers.
Don’t save data when the player joins, there is no point.
Use a dictionary instead of arrays for player data.
Do not wait for arbitrary time in your code.
The cop-out is to just use DataStore2, which I assume gets rid of some or all of these problems. I don’t like it, it is wasteful. Writing your own code for this is easy.
First of all, you’re yielding in BindToClose which is not possible. This won’t have any effect on data, but having a wait(3) at the end is pointless.
Secondly, you are doing GetAsync, checking if the value is nil, then SetAsync but this is exactly what UpdateAsync is for. You should be using UpdateAsync and checking if the old value is nil, otherwise you are wasting resources for absolutely no reason.
Thirdly, you are creating the empty table, objData, and then setting the value in UpdateAsync to objData, as well as using SetAsync with objData. Instead, you should be merging oldValue with objData by looping through objData and setting each value on oldData. What you’re doing is bound to delete data if the pets object you’re looping through gets deleted, which in the case of a player leaving, or the game being shutdown this is almost guaranteed.
Given a statement “X is not possible”, this means that you cannot do X. If you can in fact do X (yield in BindToClose), this means it is possible to yield in BindToClose.
To be honest it’s not bad code, it just has some flaws. I struggle with datastores constantly and I’ve had 10 years of experience on Roblox, so datastores are honestly not simple to work with.
Ok, after a week of mucking about with this code and trying to address your suggestions I’m not getting anywhere fast. So I’m thinking about sending the code to the market and putting a bounty on these fixes.
What price should I put on this piece of code? Although I am seriously thinking of requesting a Datastore2 conversion as part of the requirements.
It just needs to be robust. (not award winning code)
Your advice is appreciated. Thanking you in advance.
