Thoughts on my data module?

Help and Feedback Code Review
#1

I wrote a little data module for my game that allows you to directly mutate the table instead of using a Set function. It handles loading, saving, and adding defaults to already existing tables. I’m not sure of how I should improve the code. Would it be better to use a function like for setting and getting data compared to directly mutating the table? Are there any memory leaks that could break this?

Code

local DataModule = {}

local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local t = require(ReplicatedStorage.t)
local inspect = require(ReplicatedStorage.inspect)
local DataStoreService = game:GetService("DataStoreService")
local PlayerDataStore = DataStoreService:GetDataStore("PlayerData")

local MAX_RETRIES = 5

function DataModule:GetDefaultData()
	return {
		DataId = 0,
		Clips = 0,
		LifetimeClips = 0,
		Rebirth = 0,
		Coins = 0,
		Pets = {},
		EquippedPets = {}
	}
end

DataModule._PlayerData = {} -- Key is always the Player instance

--[[**
	
	Makes a DataStoreService request with automatic error handling and retry.
	
	@param dataStore the GobalDataStore instance
	
	@param method The method to use
	
	@param maxRetries The maximum amount of retries before erroring
	
	@param key The key to use
	
	@param value The value to use if needed
	
	@returns The response
	
**--]]

function DataModule.MakeRequest(dataStore, method, maxRetries, key, value)
	local success, res
	local retries = 0
	
	repeat
		if retries > 1 then
			wait(retries/10)
		end
		success, res = pcall(dataStore[method], dataStore, key, value)
		retries = retries + 1
	until success == true or retries >= maxRetries
	
	if success == true then
		return res
	else
		error(("Failed to make data store request. Data Store: %s ||| Method: %s ||| Max Retries: %i ||| Error: %s"):format(dataStore.Name, method, maxRetries, res))
	end
end

--[[**

	Loads a player's data table into the cache from the DataStoreService. Can yield.
	
	@param player The player
	
	@param force Forces data to be reloaded
	
	@returns void

**--]]

function DataModule:LoadPlayerDataTable(player, force)
	assert(t.instance("Player")(player))
	assert(t.optional(t.boolean)(force))
	
	if not force then force = false end
	
	assert(not DataModule._PlayerData[player] and force == false, "Player data is already loaded. Use the force parameter to force loading.")
	
	print("Loading player data: ", player.UserId)
	
	local success, value = pcall(DataModule.MakeRequest, PlayerDataStore, "GetAsync", math.huge, player.UserId)
	
	if success then
		-- Add default values for non-existent fields
		
		value = value or {}
		
		local defaultData = DataModule:GetDefaultData()
		
		for i, v in pairs(defaultData) do
			if not value[i] then
				value[i] = v
			end
		end
		
		print(inspect.inspect(value))
		
		DataModule._PlayerData[player] = value
	else
		error("Failed to load data. Error: " .. value)
	end
end

--[[**
	
	Returns the player data table that can be directly mutated. Can yield.
	
	@param player The player
	
	@returns table A refrence to the player's data table
	
**--]]

function DataModule:GetPlayerDataTable(player)
	assert(t.instance("Player")(player))
	-- Load player data if it isn't already loaded
	
	print("Getting player data: ", player.UserId)
	
	if not DataModule._PlayerData[player] then
		DataModule:LoadPlayerDataTable(player)
	end
	
	return DataModule._PlayerData[player]
end

--[[**
	
	Saves the player's data table to the DataStoreService. 
	
	@param player The player
	
	@returns void
	
**--]]

function DataModule:SavePlayerDataTable(player)
	assert(t.instance("Player")(player))
	
	assert(DataModule._PlayerData[player], "The player does not have a data table loaded.")
	
	local currentData = DataModule:GetPlayerDataTable(player)
	
	print("Saving player data: ", player.UserId)
	print(inspect.inspect(currentData))
	
	local success, value = DataModule.MakeRequest(PlayerDataStore, "UpdateAsync", 5, player.UserId, function(oldValue)
		local previousData = oldValue or DataModule:GetDefaultData()
		if currentData.DataId == previousData.DataId then
			print("DataIds match! Saving data.")
			currentData.DataId = currentData.DataId + 1
			print(inspect.inspect(currentData))
			return currentData
		else
			warn("Save aborted. DataIds are different.")
			return nil
		end
	end)
end

--[[**
	
	Removes the player's data from the cache to prevent memory leaks. 
	
**--]]

function DataModule:RemovePlayerDataTable(player)
	DataModule._PlayerData[player] = nil
end

return DataModule
2 Likes
#2

I don’t see a reason to expose this method. Why not make it local?
function DataModule:GetDefaultData()

1 Like
#3

I could make it local. I just had it exposed originally in case if it’s needed, but if that becomes an issue I can add it back to the table.

1 Like