DataStore Scheme

Help and Feedback Code Review
#1

Hello,

I made a datastore script that saves user’s cash every 60 seconds. I’m wondering if my code is proper enough for most of the possible scenarios.

Code:

local Data = game:GetService(“DataStoreService”)
local CashDataStore = Data:GetDataStore(“CashDS”)

game.Players.PlayerAdded:Connect(function(player)
local leaderstats = Instance.new(“Folder”, player)
leaderstats.Name = “leaderstats”

local Cash = Instance.new(“IntValue”, leaderstats)
Cash.Name = “Cash”

local data

local success, errormessage = pcall(function()
CashDataStore:GetAsync(player.UserId…“_Cash”)
end)

if success then
Cash.Value = data
else
print(“There was an error while receiving your data.”)
warn(errormessage)
end

while wait(60) do
local success, errormessage = pcall(function()
CashDataStore:SetAsync(player.UserId…“_Cash”, player.leaderstats.Cash.Value)
end)

  if success then
  	print("Data automatically saved")
  else
  	print("There was an error while auto-saving the user's data ")
  	warn(errormessage)
  end

end

end)

game.Players.PlayerRemoving:Connect(function(player)
local success, errormessage = pcall(function()
CashDataStore:SetAsync(player.UserId…“_Cash”, player.leaderstats.Cash.Value)
end)

if success then
print(“Data successfully saved upon player removal”)
else
print(“There was an error while saving the user’s data upon removal.”)
warn(errormessage)
end
end)

Any feedback would me much appreciated.

#2

Instead of just saying “There was an error while saving the user’s data upon removal.”, you should retry saving it until it works (or maximum 3 attempts if you want). If it fails loading, add a waiting screen for the user while his cash don’t loads.

1 Like
#3

Hello! I’ve spotted a few discouraged practices that you might want to fix:

  1. Using the second parameter of Instance.new(), setting the parent before the properties:
local leaderstats = Instance.new(“Folder”, player)
leaderstats.Name = “leaderstats”

local Cash = Instance.new(“IntValue”, leaderstats)
Cash.Name = “Cash”

It’s far more expensive to modify an already replicated object than setting its properties before setting its parent. When creating a new object, it’s best to do everything while it’s still just memory. I recommend you go check out these reads:

For your current code, I’d change it to this:

local leaderstats = Instance.new(“Folder”)
leaderstats.Name = “leaderstats”
local Cash = Instance.new(“IntValue”, leaderstats)
Cash.Name = “Cash”
leaderstats.Parent = player
  1. Use UpdateAsync() instead of SetAsync():

From this great PSA by ForeverHD: SetAsync completely overwrites data. This can be an issue if the save messes up, and you have no way to verify and do checks with data. UpdateAsync respects the previous entry and gives you the ability to define a callback:

Hope this helps! :slight_smile:

3 Likes
#4

Seems like a nice idea, I’ll do something similar to that. Thanks

#5

Thank you so much, especially about the second advice.

1 Like
#6

If the player fails to get their data, you need to lock the data. Otherwise it will be overwrited from the autosaving eventually.

1 Like
#7

Consider use the datastore2, this can help you a lot more. And, it handles the automatic save (every x seconds/minutes save this and this) if I remember good. Anyways, it‘s a good resource to look in!

2 Likes
#8

I noticed that you never changed the “data” variable, instead you just said:

CashDataStore:GetAsync(player.UserId…"_Cash") --this line doesn't do anything, you need to change data variable

Try instead:

local data
local success, errormessage = pcall(function()
data = CashDataStore:GetAsync(player.UserId…"_Cash")
end)

if success and data then
Cash.Value = data
else
print(“There was an error while receiving your data.”)
warn(errormessage)
end
2 Likes
#9

That’s an uneeded data variable, you can just use the returned result of pcall. Furthermore, you can skip seperately defining behavior on the else statement entirely, as shown here:

local status, result = xpcall(CashDataStore.GetAsync, warn, CashDataStore, player.UserId)

if status then blah end

Also worth mentioning you can change warn to a function literal if you need proper behavior like locking data when it fails to load to prevent overwrition.

1 Like