Is my datastore script good?

Help and Feedback Code Review
,
1

i want to know if my datastore script is good and how can it be improved
heres my script:

local ds = game:GetService("DataStoreService")
local d = ds:GetDataStore("data")
local d1 = ds:GetDataStore("dataold")
local tosave,tosaveold,saved

game.Players.PlayerAdded:Connect(function(p)
	local f1 = Instance.new("Folder")
	f1.Name = "leaderstats"
	f1.Parent = p
	
	local v1 = Instance.new("StringValue",p.leaderstats)
	v1.Name = "Kills"
	local v2 = Instance.new("StringValue",p.leaderstats)
	v2.Name = "Money"
	local v3 = Instance.new("BoolValue",game.ServerStorage.bans)
	v3.Name = p.UserId
	local v4 = Instance.new("IntValue",game.ServerStorage.warns)
	v4.Name = p.UserId
	local v5 = Instance.new("StringValue",game.ServerStorage.banreasons)
	v5.Name = p.UserId
	local v6 = Instance.new("BoolValue",game.ServerStorage.admins)
	v6.Name = p.UserId
	
	local tries = 0
	
	repeat 
		local data 
		local success, errormessage = pcall(function()
			data = d:GetAsync(p.UserId)
		end)
		if success then 
			v1.Value = data[1] or "0"
			v2.Value = data[2] or "0"
			v3.Value = data[3] or false
			v4.Value = data[4] or 0
			v5.Value = data[5] or ""
			v6.Value = data[6] or false
		else
			print("data error")
			warn(errormessage)
			p:Kick("Kicked to avoid data loss. Error message: "..errormessage)
		end
		tries=tries+1 
		print(tries)
		until data[1]~=d1:GetAsync(p.UserId)[1] or tries==3
	
	tosaveold={
		p.leaderstats.Kills.Value,
		p.leaderstats.Money.Value,
		nil,
		nil,
		nil,
		game.ServerStorage.admins[p.UserId].Value,
	}
	
	if v4.Value>=4 then
		v3.Value=true
		p:Kick("You have been banned. Reason: "..v5.Value)
	elseif v3.Value==true then
		p:Kick("You have been banned. Reason: "..v5.Value)
	end
end)

game:BindToClose(function()
	for _,p in pairs(game.Players:GetPlayers()) do
		tosave={
			p.leaderstats.Kills.Value,
			p.leaderstats.Money.Value,
			game.ServerStorage.bans[p.UserId].Value,
			game.ServerStorage.warns[p.UserId].Value,
			game.ServerStorage.banreasons[p.UserId].Value,
			game.ServerStorage.admins[p.UserId].Value,
		}

		local success, errormessage = pcall(function()
			d:SetAsync(p.UserId, tosave)
			d1:SetAsync(p.UserId, tosaveold)
			for i=1,#tosaveold do
				print("olddata "..'"'..tostring(tosaveold[i])..'"')
			end
			for i=1,#tosave do
				print("newdata "..'"'..tostring(tosave[i])..'"')
			end
		end)
		if success then
			print("saved")
			saved=true
		else
			print("save error")
			warn(errormessage)
		end
	end
end)

game.Players.PlayerRemoving:Connect(function(p)
	if saved then else
		tosave={
			p.leaderstats.Kills.Value,
			p.leaderstats.Money.Value,
			game.ServerStorage.bans[p.UserId].Value,
			game.ServerStorage.warns[p.UserId].Value,
			game.ServerStorage.banreasons[p.UserId].Value,
			game.ServerStorage.admins[p.UserId].Value,
		}
		
		local success, errormessage = pcall(function()
			d:SetAsync(p.UserId, tosave)
			d1:SetAsync(p.UserId, tosaveold)
			for i=1,#tosaveold do
				print("olddata "..'"'..tostring(tosaveold[i])..'"')
			end
			for i=1,#tosave do
				print("newdata "..'"'..tostring(tosave[i])..'"')
			end
		end)
		if success then
			print("saved")
		else
			print("save error")
			warn(errormessage)
		end
	end
	
end)
2

I would start by saying that you will have data loss if it fails to load something due to your or statements ‘v1.Value = data[1] or “0”’ which will override data should it still run.

Also you are using 2 sets of datastores which means you are saving twice as much. Why don’t you create a new save and leave the old data be so it reduces the usage.

However you are on the right track with using arrays for this but I would recommend turning it all into string to reduce the failure rate. Of course arrays are far better for sorting data so you can use table.concat() to combine it into string and string.split() to turn it back into a list.

So far I think you have done good its just a few bits to finish it up.

A summary:

  • Save as string and turn it back into a table to reduce failure rates
  • Create new stores rather than overriding every time to half dss usage (Clear old saves every week or so)
  • Log errors and put them in a datastore when the server comes to a close so you can track what’s going on
  • Don’t use or statements when loading or saving data
1 Like
3

I would start by saying that you will have data loss if it fails to load something due to your or statements ‘v1.Value = data[1] or “0”’ which will override data should it still run.

No, there will be no data loss based on those statements. The values are set when the data has loaded. Those statements are just if there was no data saved (when GetAsync successfully returns the data), so they are set to the default values. If the data fails to load, the values aren’t set to the default. However, OP would still be saving the default data (if actual data fails to load). But what you mentioned isn’t the case.

  • Save as string and turn it back into a table to reduce failure rates

That won’t do anything to reduce failure rates of GetAsync calls.

  • Don’t use or statements when loading or saving data

…

1 Like