Is my data script good?

This is to save player data, I just want to know if it’s gonna stay working and not wipe player data.
Did I do anything poorly?
Is anything just waiting to break?
Will I have dataloss?

Any tips or improvements?
Thanks for your time!

Script:

local DataStoreService = game:GetService("DataStoreService")
local DataStore = DataStoreService:GetDataStore("Donations")
local MarketPlaceService = game:GetService("MarketplaceService")

local players = game:GetService("Players")
local Tries = 4

-- Functions


local function SaveData(plr,PlayerDataFolder)
	print("SAving")
	local PlrData = {}

	PlrData.DonationsTotal = PlayerDataFolder.DonationsTotal.Value
	PlrData.Bought = plr.leaderstats.Bought.Value
	PlrData.MinutesInGame = PlayerDataFolder.MinutesInGame.Value
	
	local succes, errormsg = pcall(function()
		if plr.Loaded.Value == true then
			if PlrData ~= nil then
				DataStore:SetAsync(plr.UserId,PlrData)
			else 
				print("No data, Cancelled save")
			end	
		end
	end)
	if succes then
		print("Saved")
	elseif errormsg then
		print(errormsg)
	end
end

local function GetData(plr,PlayerDataFolder)
	local data
	local count = 0
	local succes, errormsg
	repeat 
		succes, errormsg = pcall(function()

			data = DataStore:GetAsync(plr.UserId)

		end)
		count += 1
	until count >= Tries or succes
	if succes then
		print("Succes")
	elseif errormsg then
		print(errormsg)
	end
	if not succes then
		plr:Kick("A fatal Datastore error occured and You have been kicked to prevent data loss, Try rejoining")
	end
	if succes then
		if data ~= nil then
			if data.DonationsTotal then
				PlayerDataFolder.DonationsTotal.Value = data.DonationsTotal
			end
			if data.Bought then
				plr.leaderstats.Bought.Value = data.Bought
			end
			if data.MinutesInGame then
				PlayerDataFolder.MinutesInGame.Value = data.MinutesInGame
			end
		else
			print("No data")
		end
		plr.Loaded.Value = true
	else
		print("ERROR")
	end
end


-- Main Setup
game.Players.PlayerAdded:Connect(function(plr)
	-- Main Data Storage Folder
	local LoadedBool = Instance.new("BoolValue",plr)
	LoadedBool.Value = false
	LoadedBool.Name = "Loaded"
	
	local PlayerDataFolder = Instance.new("Folder")
	PlayerDataFolder.Name = "Data"
	PlayerDataFolder.Parent = plr
	
	local leaderstats = Instance.new("Folder")
	leaderstats.Name = "leaderstats"
	leaderstats.Parent = plr

	local DonationsTotal = Instance.new("IntValue",PlayerDataFolder)
	DonationsTotal.Value = 0
	DonationsTotal.Name = "DonationsTotal"
	
	local Bought = Instance.new("IntValue",leaderstats)
	Bought.Value = 0
	Bought.Name = "Bought"
	
	local MinutesInGame = Instance.new("IntValue",PlayerDataFolder)
	MinutesInGame.Value = 0
	MinutesInGame.Name = "MinutesInGame"
	
	GetData(plr,PlayerDataFolder)
end)

game.Players.PlayerRemoving:Connect(function(plr)
	if plr:FindFirstChild("Loaded") then
		if plr.Loaded.Value == true then
			SaveData(plr,plr.Data)
		end
	end
end)
game:BindToClose(function()
	for i, plr in pairs(game.Players:GetChildren()) do
		if plr:FindFirstChild("Loaded") then
			if plr.Loaded.Value == true then
				SaveData(plr,plr.Data)
			end
		end
	end
end)

DataStore.lua (2.9 KB)

1. Messy statements
I see a lot of unnecessarily complicated statements, for example here:

    if succes then
		print("Succes")
	elseif errormsg then
		print(errormsg)
	end
	if not succes then
		plr:Kick("A fatal Datastore error occured and You have been kicked to prevent data loss, Try rejoining")
	end
	if succes then
		if data ~= nil then

It would be way more clean if you replaced it with:

    if not succes then
        print(errormsg)
        return plr:Kick("A fatal Datastore error occured and You have been kicked to prevent data loss, Try rejoining")	
    end

    -- if the code reached this point, it means the success is true
    if data ~= nil then

Also, instead of making a “pyramid” of ifs, you could use logical operators.

if succes then
	if data ~= nil then

-- can be replaced with

if success and data ~= nil then

Btw. it’s success, not succes :slight_smile:

2. Second argument of Instance.new constructor
Parenting stuff using second argument of Instance.new() is considered a bad practice.

local a = Instance.new('IntValue', workspace)
-- is slower than
local a = Instance.new('IntValue')
a.Parent = workspace

3. Data safety
Although your DataStore script seems safe, I recommend you to use a “back-up” DataStore as well.
If the data was saved successfuly to the main DataStore, save them to the back-up DataStore, and then if there’s some error when getting the main data, you can always use the back-up.

4. Using loops
I don’t really like this part of your code where you create the values.
It’s not a big deal but personally, I would rather use a table for all the values and then add them automatically using a loop. Something like this:

local values = {
    ['DonationsTotal'] = {'IntValue', 'Data', 0},
    ['Bought'] = {'IntValue', 'leaderstats', 0},
    ['MinutesInGame'] = {'IntValue', 'Data', 0}
}

Players.PlayerAdded:Connect(function(plr)
    for name, info in next, values do
        local inst = Instance.new(info[1])
        inst.Name = name
        inst.Parent = plr[info[2]]
        inst.Value = info[3]
    end
end)

Hope it helps, wishing you all the best in your future development!

3 Likes

Thanks!
a lot of the if success’ is leftover from debugging lol.
(I can’t believe I typed success lol, I code way too late at night XD)
I had no idea using the second argument of instance.new() was slower Good thing to know.
The backup datastore seems like an amazing idea!
And that table for creating the new values looks so much neater, I’m definitely going to try that.