How could I improve the neatness and efficiency of my DataStore code?

Help and Feedback Code Review
#1

Hello! So the past few days I’ve been working on DataStores and I finally got them working…but my code looks a little bit sloppy from what I’ve seen others do. Does anyone have any tips on how I could improve my DataStore code? Either just better coding practices, or changing how I do some of the code.

Any tips/feedback would be much appreciated!!

local Module = {}
local DataStoreService = game:GetService('DataStoreService')
local DataStore = DataStoreService:GetDataStore("Player_Data_Test22")

local SessionData = {
	HighestLevel = {}, -- Format = 124828424_HighestLevel
	OtherStat = {} -- Format = 124828424_OtherStat
}

local Defaults = {
	HighestLevel = 1,
	OtherStat = 10
}

local CanSave = {--[[userID = true]]}

--[[ LOADING STATS WHEN PLAYER JOINS ]]
game.Players.PlayerAdded:connect(function(Player)	
	local UserId = Player.UserId
	local Key = "player-" .. (tostring(UserId))
	local PlayerData; 	
	
	local DataFetchSuccess, ErrorMessage = pcall(function()                                                      
    	PlayerData = DataStore:GetAsync(Key)
	end)
	
	if DataFetchSuccess then 
		print("success")
		if PlayerData ~= nil then 
			print("~= nil")
			local StatIndex = 1
			for Stat, Value in pairs(SessionData) do
				print("indexing")
    			SessionData[Stat][UserId .. "_" .. Stat] = PlayerData[StatIndex]
				StatIndex = StatIndex + 1
			end
		else 
			print("== nil")
			for Stat, Value in pairs(Defaults) do
				print("indexing")
    			SessionData[Stat][UserId .. "_" .. Stat] = Defaults[Stat]
			end
		end
				
	else 
		CanSave[UserId] = false
		Player:Kick("Your data failed to load! Please rejoin.")				
	end  
end)


--[[ SAVING STATS WHEN PLAYER LEAVES ]]
game.Players.PlayerRemoving:connect(function(Player)
	local UserId = Player.UserId
	local Key = "player-" .. (tostring(UserId))
	
	if CanSave[UserId] == false then return end 
	
	local ValuesToSave = {}
	for Stat, Value in pairs(SessionData) do
        table.insert(ValuesToSave,SessionData[Stat][UserId .. "_" .. Stat])
    end

	local DataSaveSuccess, ErrorMessage = pcall(function()
		print("Stats: HighestLevel, OtherStat: " .. game:GetService([[HttpService]]):JSONEncode(ValuesToSave))
		DataStore:SetAsync(Key, ValuesToSave)
	end)	
	
	if not DataSaveSuccess then 		
		local RetryCount = 0
		
		while RetryCount < 4 do
			wait(60)
			local Success, Error = pcall(function()
    			DataStore:SetAsync(Key, ValuesToSave)
			end)
			if Success then 
				break 
			end
			RetryCount = RetryCount + 1
		end	
	end
	
	--Clearing Stats
	CanSave[UserId] = nil
	SessionData["HighestLevel"][UserId .. "_HighestLevel"] = nil
	SessionData["OtherStat"][UserId .. "_OtherStat"] = nil
end)

return Module
4 Likes
#2

It’s been a while since I’ve hankered down to Code Review. That aside, I do have some suggestions in terms of improving the neatness of your code.

Using pcalls correctly

As opposed to wrapping an anonymous function and setting an upvalue, you can wrap the actual call itself in one swoop.

-- Previous
local PlayerData;

local DataFetchSuccess, ErrorMessage = pcall(function()
    PlayerData = DataStore:GetAsync(Key)
end)

-- New
local DataFetchSuccess, CallResult = pcall(DataStore.GetAsync, DataStore, Key)

Why?

This, as I mentioned earlier, wraps the call itself instead of an anonymous function. This is efficient, readable and maintainable use of a pcall.

Protected call accepts two parameters - the function and the arguments. The function is wrapped and the arguments specified are passed to the function. In this case, we wrap GetAsync.

The equivalent of DataStore:GetAsync(Key) is DataStore.GetAsync(DataStore, Key). You can even try this in Studio and it will work.

Roblox objects are object-oriented so when you call a method with colon syntax, you pass the object to itself. In this case, the parameters of GetAsync are self and Key. Self is hidden and you’re only expected to pass key.

Add retries

Kicking a player if their data can’t be loaded is usually bad for UX, since you want to aim to interrupt a player’s experience as little as possible (especially if your audience is young).

How?

Each player should be given around 2-3 retries for fetching data before you do anything. Personally, I wouldn’t even kick them either - I’d probably just use the TeleportService to effectively have them “rejoin”.

In terms of retries, the most you’d need is a function that calls a pcalled DataStore method (GetAsync) until either a success value is returned or the amount of retries has been reached. I believe there’s a code sample on the Developer Hub somewhere about this.

You somehow seem to have nailed retries for saving though. All you need is to do the same for fetching.

Usage of SetAsync

Essentially, my whole point is contained in a resource article regarding using SetAsync over UpdateAsync. I don’t want to reiterate, so I’ll show you the thread instead. You can determine if you want to keep it or change after reading, if you want to read it.

Thread

These are some immediate concerns that I can take up about your code, as well as suggestions I can make without going against the guides of this category (which are quite strict).

The rest of your code looks completely fine. Awesome work out there.

16 Likes
#3

Awesome! Thanks so much for all of your tips. I’ll be switching things around later today. I do have one question though.

If I run the pcall as you suggest, I’ll lose my PlayerData value, which I need later on in the script.

if PlayerData ~= nil then 
	print("~= nil")
	local StatIndex = 1
	for Stat, Value in pairs(SessionData) do
		print("indexing")
    	SessionData[Stat][UserId .. "_" .. Stat] = PlayerData[StatIndex]
		StatIndex = StatIndex + 1
	end
else 
	print("== nil")
	for Stat, Value in pairs(Defaults) do
		print("indexing")
    	SessionData[Stat][UserId .. "_" .. Stat] = Defaults[Stat]
	end
end

Assuming since I don’t set the PlayerData value, I can’t still easily check it? Is there another way to check whether the Data is there or not?

1 Like
#4

Actually the second argument returned by pcall is the value returned inside the pcall function. If the pcall fails, it’s the error message.

local success, value = pcall(function()
   return 32
end)

print(success, value)
> true 32

Thus, CallResult in @colbert2677’s code would be the equivalent to your PlayerData. You could always rename it!

6 Likes
#5

I added in the loop for the retries, but I don’t actually set the SessionData in that loop, so I’m going to end up with a ton of errors when accessing the data. Would it be best to create a function for loading the data, or should I copy and paste what’s in the if DataFetchSuccess then to the if not DataFetchSuccess loop?

Or am I completely over thinking/complicating this?

game.Players.PlayerAdded:connect(function(Player)	
	local UserId = Player.UserId
	local Key = "player-" .. (tostring(UserId))	
	
	local DataFetchSuccess, CallResult = pcall(DataStore.GetAsync, DataStore, Key)                                                      
	
	if DataFetchSuccess then 
		print("success")
		if CallResult ~= nil then 
			print("~= nil")
			local StatIndex = 1
			for Stat, Value in pairs(SessionData) do
				print("indexing")
    			SessionData[Stat][UserId .. "_" .. Stat] = CallResult[StatIndex]
				StatIndex = StatIndex + 1
			end
		else 
			print("== nil")
			CanSave[UserId] = false
			print("teleporting")
			for Stat, Value in pairs(Defaults) do
				print("indexing")
    			SessionData[Stat][UserId .. "_" .. Stat] = Defaults[Stat]
			end
			wait(3)
			game:GetService("TeleportService"):Teleport(2111151427, Player)	
		end
				
	elseif not DataFetchSuccess then 		
		local RetryCount = 0
		
		while RetryCount < 4 do
			wait(60)
			local Success, CallResult = pcall(DataStore.GetAsync, DataStore, Key) 
			
			if not Success then 
				CanSave[UserId] = false 
				--Player:Kick("Your data failed to load! Please rejoin.")
			else 
				break			
			end
		end					
	end  
end)
1 Like
#6

Might be overthinking it a little.

You may want to get rid of the loop entirely and opt for a retry function. This would wrap any DataStore functions and return results. You could then embed the retry function to be used for any DataStore.

Considering that DataFetchSuccess and CallResult exist as upvalues to the loop, you can get rid of the “local” part and account for failed fetches before setting/interacting with data.

local DataFetchSuccess, CallResult = pcall(DataStore.GetAsync, DataStore, Key)

if not DataFetchSuccess then
    local RetryCount = 0

    while RetryCount < 4 or not DataFetchSuccess do
        DataFetchSuccess, CallResult = pcall(DataStore.GetAsync, DataStore, Key)
        if not DataFetchSuccess then
            wait(1) -- Yield if not successful, otherwise let next iteration start immediately (which terminates as condition passes)
        end
    end
end

if DataFetchSuccess then
    if CallResult then
        -- Handle CallResult
    else
        -- Handle nil CallResult
    end
else
    -- Handle error
end
1 Like