Datastore Script Review

Hey developers.

Please do not be afraid to eat me alive with this post, I’d like to make sure I get this right.

I am making a datastore script, as you do, that only saves a players data if it is different from the data that is already saved.

Here are some specifics that I was wondering about:

  • Is there a better way to store all the data than an array? Perhaps a module?
  • Is my method of comparing the arrays reliable?
  • What are the chances of data loss? How can I prevent this?

If there is anything else you see that can be improved let me know in the replies. :slight_smile:

local Datastore = game:GetService("DataStoreService"):GetDataStore("MainDatastore11")
local DefaultValues = {
	["Inventory"] = {"Default"},
	["Wins"] = 0,
	["Coins"] = 100,
	["Codes"] = {},
	["Equipped"] = "Default",
	["GamepassesReceived"] = {},
}

local Data = {}

game.Players.PlayerAdded:Connect(function(Player)
	local NewData = Datastore:GetAsync(Player.UserId^135) or {}

	for ValueName, Value in pairs(DefaultValues) do
		if not NewData[ValueName] then
			NewData[ValueName] = Value
		end
	end

	Data[Player.UserId] = NewData

-- Originally there was a value.Changed event for updating leaderstats in the data variable
end)

game.Players.PlayerRemoving:Connect(function(Player)
	--[Datastore]--
	if Data[Player.UserId] then

		local ShouldSave = false

		local OldData = Datastore:GetAsync(Player.UserId^135) or {}

		for Value, DV in pairs(DefaultValues) do
			if type(DV) == "table" then
				for I, Val in pairs(Data[Player.UserId][Value]) do
					if (not OldData[Value]) or (not OldData[Value][I]) or (Val ~= OldData[Value][I]) then
						ShouldSave = true
					end
				end
			else
				if OldData[Value] ~= Data[Player.UserId][Value] then
					ShouldSave = true
				end
			end
		end

		if ShouldSave then
			local Count = 1
			repeat
				local Success, ErrorMessage = pcall(function()
					Datastore:SetAsync(Player.UserId^135, Data[Player.UserId])
				end)
				if Success then
					-- Saved successfully
					Data[Player.UserId] = nil
					break
				else
					-- Hit error saving.

					Count = Count + 1

					wait(5)
				end
			until Success or Count == 5
		else
			-- Data not changed.
		end
	else
		-- No data was found.
	end
end)

(comments in the saving script were being used for logging but I removed them for this post :D)

4 Likes

That looks perfectly fine to me and it seems to have a great use of tables.

Are there any things you think that I can improve on w/ this script?

In if else statement of checking success you should do:

warn(ErrorMessage);

So if an error occurs you can see it in output.

Yup.
Originally I had discord logging in the script, but I removed it before positng it here.

If I end up using this script again, I’ll probably add warn.

Wouldn’t recommend you to use discord for logging warnings/error kind of things. Also you should use pcalls on all datastore calls I believe :slight_smile:

Why would discord logging be bad for logging errors? When I’ve worked with big games, they always require logging incase of data loss so that they can see if the data failed etc.

They have API Rate limits. So just need to keep that in mind, but I still won’t recommend because once the game gets big there may be more fails than normally are and you may eventually cross the request Limit.

I would just recommend implementing a strong datastore system, that can handle everything incase of Data save failures, such as a Retry system etc.

1 Like

Haha, exactly the reason for making this post.

I have a retry system in the script (see op)… do you think that this can be improved?

The logging system that I made can send all the messages without crossing the HTTPS limit.

Also the requests are per minute I believe, so it’s not really a gradual thing.

Oh, sorry, I never actually saw the Original post properly, but now after seeing it feels fine although you might want to make the wait for about 7 seconds instead, as the wait time between Datastore write methods is 6.

Another thing that I notice is, you should be used pcalls on all datastore calls, with that I mean on GetAsync too, just incase data was unable to be loaded, you probably won’t want the player to keep continue playing as it may later overwrite their data.

Last thing is, that you can use pcalls in a much shorter way, I mean what you’re doing is putting a function inside the pcall and doing datastore method calls inside that anonymous function. Instead an example of using it properly (it works in your way too, this just makes it much more readable):

local success, response = pcall(Datastore.SetAsync, Datastore, Player.UserId^135, Data[Player.UserId])

The way pcalls work is, they first accept the function/method that is to be used, secondly they accept the Object, that the method is to be used on, basically self in OOP. Then the rest are the arguments for the function.


Also you should implement BindToClose too, as it saves you from alot of dataloss on server shutdown/crash events etc.

First of all

local NewData = Datastore:GetAsync(Player.UserId^135) or {}

wrap that in a pcall, GlobalDataStore:GetAsync() fails too.

Second of all, it’s best to use DataModel:BindToClose() to also save data, since if the server shuts down, the player’s data my not save in time. You should also add an autosave system, since bound functions don’t run when the server crashes. The autosave interval should be a few minutes.

Lastly, this line

local OldData = Datastore:GetAsync(Player.UserId^135) or {}

I don’t see any reason to do this, it’s just adding an extra step, rather than just saving the data.

Hey there!

I agree with most of what you’ve said here… but I had a couple of questions.

How would this actually look in the code? Should I just replace SetAsync with BindToClose?

The reason for getting the player’s old data was to compare it with what the server has in terms of the player’s data.

If the OldData is not equal to what is trying to be saved then it will save.

Thanks so much fo the input!

The code for BindToClose would look like

game:BindToClose(function()
    for i, plr in pairs(game.Players:GetPlayers()) do
        -- save the data here
    end
end)

Also, BindToClose only runs when the game closes, you need to use PlayerRemoving for saving when a player leaves (you may need to add another function that saves data so you can just call that function in BindToClose and PlayerRemoving).

1 Like

Wrapping the save function inside the BindToClose function in a coroutine is needed as saving for one player can take upto 1 second, and if there are about 30-45 players, they could face a data loss for the last 10-15 players.

Thanks so much for this, I already have a player removed function. I should spawn the function inside the BindToClose, right? spawn(function() save() end)

Don’t use spawn(), use coroutine.wrap() instead, spawn() has a delay (the same as wait()), it would look like this instead

game:BindToClose(function()
    for i, plr in pairs(game.Players:GetPlayers()) do
        coroutine.wrap(save)(plr)
    end
end)
1 Like

Hey all!

I have taken all suggestions into consideration, and here is my new and improved script:

local Datastore = game:GetService("DataStoreService"):GetGlobalDataStore("MainDatastore11")
local DefaultValues = {
	["Inventory"] = {"Default"},
	["Wins"] = 0,
	["Coins"] = 100,
	["Codes"] = {},
	["Equipped"] = "Default",
	["GamepassesReceived"] = {},
}

local Data = {}

game.Players.PlayerAdded:Connect(function(Player)
	local Count = 0
	local Success
	
	local NewData
	
	repeat
		local Success, Failure = pcall(function()
			NewData = Datastore:GetAsync(Player.UserId^135) or {}
		end)
		
		if not Success then wait(5) Count = Count + 1 end
		
	until Success or Count == 5
	
	if Count == 5 then Player:Kick("Unable to retrieve data! Please try again later.") end

	for ValueName, Value in pairs(DefaultValues) do
		if not NewData[ValueName] then
			NewData[ValueName] = Value
		end
	end

	Data[Player.UserId] = NewData

	-- Originally there was a value.Changed event for updating leaderstats in the data variable
end)

function Save(Player)
	--[Datastore]--
	if Data[Player.UserId] then

		local ShouldSave = false

		local OldData = Datastore:GetAsync(Player.UserId^135) or {}

		for Value, DV in pairs(DefaultValues) do
			if type(DV) == "table" then
				for I, Val in pairs(Data[Player.UserId][Value]) do
					if (not OldData[Value]) or (not OldData[Value][I]) or (Val ~= OldData[Value][I]) then
						ShouldSave = true
					end
				end
			else
				if OldData[Value] ~= Data[Player.UserId][Value] then
					ShouldSave = true
				end
			end
		end

		if ShouldSave then
			local Count = 1
			repeat
				local Success, ErrorMessage = pcall(function()
					Datastore:SetAsync(Player.UserId^135, Data[Player.UserId])
				end)
				if Success then
					-- Saved successfully
					Data[Player.UserId] = nil
					break
				else
					-- Hit error saving.

					Count = Count + 1

					wait(5)
				end
			until Success or Count == 5
		else
			-- Data not changed.
		end
	else
		-- No data was found.
	end
end

game.Players.PlayerRemoving:Connect(Save)

game:BindToClose(function()
	for _, Player in pairs(game.Players:GetPlayers()) do
		coroutine.wrap(Save(Player))
	end
end)

Does anyone have further suggestions?

Your wrapping the function in the coroutine incorrectly, do it like how @FastKnight401 showed you.

1 Like

I don’t understand why you would use the player user id’s to the power of 135. Squaring or cubing it is fine but to the power of 135? That’ll surley cause a little bit of lag.

You should also try to store the data in a Json encrypted Table using httpservice:JsonEncode(table) to store a string which will probably use less space compared to a regular LUA table. To turn it back into a Lua table, use httpservice:JsonDecode(jsonstring)

1 Like

Thats how roblox stores tables in the datastores it won’t make a difference.