Code review on my GetData function

Help and Feedback Code Review
1 

local function GetData(player)
	
	local key = string.format("-Player_Key: %s", player.UserId)
	local attempts = 0
	local data
	
	repeat wait(DATA_GET_INTERVAL)
		
		local success, errormessage = pcall(function()
			data = PlayerDataStores:GetAsync(key)
		end)
		
		if success then	
			if data then
				GlobalDataFolder[player.Name].Wins.Value = data.Wins
				GlobalDataFolder[player.Name].Coins.Value = data.Coins
			else
				GlobalDataFolder[player.Name].Wins.Value = DEFAULT_WINS
				GlobalDataFolder[player.Name].Coins.Value = DEFAULT_COINS
			end
			GlobalDataFolder[player.Name].Loaded.Value = true
			print(string.format("Data loaded for %s", player.Name))
		else
		    print(string.format("Data did not for load %s %s %s %s %s", player.Name, " | Attempts:", attempts, " | Debug Code: ", errormessage))
			attempts += 1
		end
	until attempts > MAX_ATTEMPTS or success
end

[Updated the whole script]

Note: DATA_GET_INTERVAL is 5.

This is how I handle player’s data. I wouldn’t consider it a bare bone but here is what I want reviewed.

  • How I handle the booleans.
  • How give the player the data
2 
if data then
	player.leaderstats.Level.Value = data.Level
	player.Data.Loaded.Value = true
else
	player.leaderstats.Level.Value = 1
	player.Data.Loaded.Value = true
end

Definitely move player.Data.Loaded.Value = true outside the check, because it happens in both cases and the way it is now it’s a bit confusing.

if data then
	player.leaderstats.Level.Value = data.Level
else
	player.leaderstats.Level.Value = 1
end
player.Data.Loaded.Value = true

It should probably be set after the leaderstats stuff is updated, because something that waits for the BoolValue to be set might then expect the leaderstats to also have been set, because it seems that you define “getting the player’s data” as “getting the data and updating leaderstats to reflect that”.

Speaking of which, I would keep the data-loading and leaderstat-updating completely separate. Lots of game systems are likely to want to get player data, and lots of systems are to change player data, so I think it’s more sensible to have a centralized place to update leaderstats to reflect changes to player data (like a LeaderstatsUpdater script).

if attempts >= 3 then
	player.Data.Loaded.Value = false
end

This is not a great way of signalling that player data loading has failed, because it’s going to be false by default so it’s hard to tell if it has failed or it just isn’t done yet. I would handle that by firing a BindableEvent to let every game system know (if they care) that data loading has failed, and then kick the player from the game to force them to try to rejoin.

You should also use a variable at the top of the script called MAX_LOADING_RETRIES or something instead of using a magic number 3. It’s especially bad because you’re using it in two places, you could accidentally change one and not the other.

I would move the “attempt incrementing and checking” code outside the if success then statement, because logically you’ve also made an attempt even if you succeded. It just makes the code a bit more intuitive to think about.

I would also use either > or == to see if too many attempts have been made. Using the >= comparison makes it a bit more difficult to reason about exactly how many attempts are made, and the only way that attempts can be greater than 3 anyway is if you made a bad mistake somewhere else in the code. If you want to check for that, do it separately to make it extremely clear what you’re doing and why.

I would also increment the attempts variable at the top of the repeat loop. The first time around, we’re not on “the zeroth attempt”, we’re actually on the first. Unless you have a good reason to, “the first” should be represented by the number 1. You’ve also got a bug because you’re incrementing it after the check, so it actually tries four times instead of the expected 3:

1st time around: attempts = 0
2nd time around: attempts = 1
3rd time around: attempts = 2
4th time around: attempts = 3 --The loop actually exits here

1 Like
3

As for how to pass the player data to other systems:

I would have BindableEvents and BindableFunctions inside the script that are 100% responsible for being the interface between the data loading script and any other game system. E.g. you might have a GetPlayerData BindableFunction that returns the player’s (cached) data. Don’t need a SetPlayerData, but you might want a SetPlayerStat, which sets a specific key-value pair of the player’s data (making sure this is validated by the player data script before actually doing anything).