Datastore Code Review [ Help me simplify and rectify ]

Help and Feedback Code Review
,
#1

Provide an overview of:

  • What does the code do and what are you not satisfied with?
    I’m not a huge fan of my pcall implementation for beginners, I want to teach them to use pcall() but not overwhelm them with information nor code.
  • What potential improvements have you considered?
    I’ve considered and tried a variety of methods to load and save the data with a retry process but so far haven’t come to a satisfactory conclusion.
  • How (specifically) do you want to improve the code?
    Simplicity is a big one as it’s going to be used by beginners getting used to roblox game development although they’ll already know how to code in lua. As such formatting is a big one so any mistakes or OOP issues please let me know. 
local DataStore = game:GetService("DataStoreService")
local Players = game:GetService("Players")
local LeaderboardMoney = DataStore:GetDataStore("LeaderboardMoney")

local function DataStore(Type,UserId,Attempts,MoneyValue)
	local key = "Player_"..UserId
	local success,data = pcall(function()
		if Type == "Fetch" then
			return LeaderboardMoney:GetAsync(key)
		elseif Type == "Save" then
			return LeaderboardMoney:SetAsync(key,MoneyValue)
		end
	end)
	if success or Attempts >= 3 then else
		Attempts = Attempts + 1 
		DataStore(Type,UserId,Attempts,MoneyValue)
	end
	return success,data
end

function PlayerJoined(plr)
	local LoadSuccess,Data = DataStore("Fetch",plr.UserId,0)
	if not LoadSuccess then return warn("DataStore | FAILED") end
	local Money = plr:WaitForChild("leaderstats"):WaitForChild("Money")
	Money.Value = Data
end
Players.PlayerAdded:Connect(PlayerJoined)

function PlayerLeaving(plr)
	local Money = plr:WaitForChild("leaderstats").Money
	local SaveSuccess,Data = DataStore("Save",plr.UserId,0,Money.Value)
	if not SaveSuccess then return warn("DataStore | FAILED") end
end
Players.PlayerRemoving:Connect(PlayerLeaving)

Please be as nit-picky as possible, I really appreciate it!

#2

I decided to remake your datastore code and make it more easier to read as well as to fix some errors I saw in your code.

One of the errors (and I think the only error I could find) was your use of pcall. pcall returns a tuple with a boolean and an error message if there is any. I don’t know why you put the data as the error message but you should probably fix that.

Keep in mind I didn’t fully test this code so there’s a chance it might not work

local dataStoreService = game:GetService("DataStoreService")
local moneyDataStore = dataStoreService:GetDataStore("LeaderboardMoney")

local function getDataStoreKey(userId)
	local KEY_PREFIX = "Player_"
	local key = KEY_PREFIX..tostring(userId)
	
	return key
end

local function dataStore(action, userId, extraData)
	local MAX_ATTEMPTS = 3
	local attempts = 0
	
	local extraData = extraData or {}
	
	local key = getDataStoreKey(userId)
	local moneyValue = extraData.moneyValue
	
	local data
	
	-- functions
	local function getData()
		return moneyDataStore:GetAsync(key)
	end
	local function setData()
		return moneyDataStore:SetAsync(key, moneyValue)
	end
	
	-- init
	local actionAssignments = {
		["set"] = setData,
		["get"] = getData
	}
	
	local success = pcall(function()
		data = actionAssignments[action]()
	end)
	
	if not success then
		repeat 
			success = pcall(function()
				data = actionAssignments[action]()
			end)
			
			attempts = attempts + 1
		until success or attempts >= MAX_ATTEMPTS
	end
	
	return success, data
end

local function onPlayerAdded(player)
	local leaderstats = player:WaitForChild("leaderstats")
	local moneyValueObject = leaderstats:WaitForChild("Money")
	
	local success, data = dataStore("get", player.UserId)
	
	if success then
		moneyValueObject.Value = data
		print("Successfully FETCHED data for: "..player.Name)
	else
		warn("Failed to FETCH data for: "..player.Name)
	end
end

local function onPlayerRemoving(player)
	local leaderstats = player:WaitForChild("leaderstats")
	local moneyValueObject = leaderstats:WaitForChild("Money")
	
	local success = dataStore("set", player.UserId, {
		moneyValue = moneyValueObject.Value
	})
		
	if success then
		print("Successfully SET data for: "..player.Name)
	else
		warn("Failed to SET data for: "..player.Name)
	end
end

game.Players.PlayerAdded:Connect(onPlayerAdded)
game.Players.PlayerRemoving:Connect(onPlayerRemoving)
#3

I was setting the Get/SetAsync data to the error variable - which would be overwritten if there was an error in which case I would check if there was an error and hence never use the overwritten info. So it works but that kind of trick (if you can call it that) isn’t helful for beginners so I’ll use your method although edited.

Thank you and let me know if there are any further discrepancies with my code from formatting, Inconsistent CaseStyles etc to bad practises etc. I appreciate it!

1 Like