How should I improve this module?

[Free_Br1cks (ren)]

For the past couple of days I’ve been trying how I could implement metamethods with DataStoreService and recently I’ve found how I could do it. Any improvements I can apply onto my module?

local Data = {}

local Players = game:GetService("Players")
local DataStoreService = game:GetService("DataStoreService")
local DATASTORE = DataStoreService:GetDataStore("data_1")

local dataProfiles = {}
local Profiles 	   = {
	--[[
	
			["51739716"] = {
				Money = 100,
				Gems = 500,
				Deaths = 50
			}
	
	]]
}

local Signal = require(script.Signal)

local activeTemplate = nil -- template that is actively being used and has already been set by :SetTemplate
--[[

		local activeTemplate = {
			Money = 100,
			Gems = 500,
			Deaths = 50
		}

]]

local templateLoaded = false

-- Private Functions

local function Save(player: Player?)
	if typeof(player) ~= "Instance" then
		error(string.format("Player expected, got %s", typeof(player)))
	end

	local success, result = pcall(function()
		DATASTORE:UpdateAsync("PLAYER_" .. player.UserId, function(oldVal)
			return Profiles[player.UserId]
		end)
	end)

	if success then
		print(string.format("Saving %s's data is a success", player.Name))
	else
		print(string.format("Unable to save %s's data, result: %s", player.Name, result))
	end
end

local function Load(player: Player?)
	if typeof(player) ~= "Instance" then
		error(string.format("Player expected, got %s", typeof(player)))
	end

	local data
	local success, result = pcall(function()
		data = DATASTORE:GetAsync("PLAYER_" .. player.UserId) or activeTemplate
	end)

	if success then
		print("Data successfully loaded for player via :GetAsync()")
		return data
	else
		player:Kick("Unable to load your data, please rejoin the game.")
		print(result)
	end
end

local function onPlayerAdded(player: Player)
	if typeof(player) ~= "Instance" then
		error(string.format("Player expected, got %s", typeof(player)))
	end
	
	if activeTemplate == nil then
		error("activeTemplate has not been loaded in")
	end
	
	local data 		   = Load(player)
	local dataProfile  = {}
	local mt 		   = {__events = {}}
	
	-- loop through given template, and add the signals
	for property, value in pairs(activeTemplate) do
		
		-- only accept properties that are numbers to have signals
		if typeof(value) == "number" then
			mt.__events[property] = Signal.new()
		end
	end
	
	mt.__newindex = function(t, k, v)
		-- if the indexed value is inside the template
		if activeTemplate[k] then
			-- if the value we're changing with is the same as the property
			if typeof(v) == typeof(activeTemplate[k]) then
				Profiles[player.UserId][k] = v
				-- if a signal exists for the property
				if mt.__events[k] then
					return mt.__events[k]:Fire(v)
				end
			else
				error(string.format("Unable to assign property %s. %s expected, got %s", k, typeof(activeTemplate[k]), typeof(v)))
			end
		else
			error(string.format("%s is not a valid member of activeTemplate", k))
		end
	end

	mt.__index = function(t, k)
		-- if the indexed value is in the template
		if activeTemplate[k] then
			-- return the player's indexed data
			return Profiles[player.UserId][k]
		else
			error(string.format("%s is not a valid member of activeTemplate", k))
		end
	end
	
	mt.__tostring = function(t)
		return "DataClass"
	end
	
	function dataProfile:GetPropertyChangedSignal(property)
		local signal = mt.__events[property]
		if signal then return signal end
	end
	
	setmetatable(dataProfile, mt)
	dataProfiles[player.UserId] = dataProfile
	Profiles[player.UserId] = data
end

local function onPlayerRemoving(player: Player)
	if typeof(player) ~= "Instance" then
		error(string.format("Player expected, got %s", typeof(player)))
	end
	
	Save(player)
	Profiles[player.UserId] = nil
	
	-- disconnect all of the signals
	for _, signal in pairs(getmetatable(dataProfiles[player.UserId]).__events) do
		signal:DisconnectAll()
	end
	
	dataProfiles[player.UserId] = nil
end

-- Main Functions

function Data:SetTemplate(template)
	if typeof(template) ~= "table" then
		string.format(error("Unable to assign 'template' %s expected, got %s", "table", typeof(template)))
	end

	activeTemplate = template
	templateLoaded = true

	Players.PlayerAdded:Connect(onPlayerAdded)
	Players.PlayerRemoving:Connect(onPlayerRemoving)
	
	-- in case if there are players who joined before this script has been executed
	for _, player in pairs(Players:GetPlayers()) do
		onPlayerAdded(player)
	end
end

function Data:GetData(player: Player)
	if typeof(player) ~= "Instance" and not player:IsA("Player") then
		error(string.format("Player expected, got %s", typeof(player)))
	end
	
	if not templateLoaded then
		error("Unable to get data if you haven't set a template yet using :SetTemplate()")
	end
	
	local profile = dataProfiles[player.UserId]
	
	if profile == nil then
		repeat
			profile = dataProfiles[player.UserId]
			task.wait(.1)
		until profile ~= nil
	else
		return profile
	end
	
	return profile
end

return Data

[BullfrogBait]

Will you eventually open-source this? If not, I’d suggest removing the excessive type checking. Especially with the PlayerAdded function. This is like micro-optimizations but just micro-checking.

Also to follow code etiquette guidelines, this:

local data 		   = Load(player)
local dataProfile  = {}
local mt 		   = {__events = {}}

Should be this:

local data = Load(player)
local dataProfile = {}
local mt = {__events = {}}

for _, player in pairs(Players:GetPlayers()) do
		onPlayerAdded(player)
	end

You don’t need to include pairs() when iterating anymore. Unless you have a specific reason to.

[Free_Br1cks (ren)]

If you don’t need pairs when iterating anymore then what should it be?

[BullfrogBait]

for _, player in Players:GetPlayers() do
	onPlayerAdded(player)
end

[kingerman88]

Important feedback

eg the stuff which can cause bad things

---

Infinite Loop

On the off chance that a player’s profile doesn’t load, and then the player leaves. This would be a spot for a potential memory leak.

@Free_Br1cks

	if profile == nil then
		repeat
			profile = dataProfiles[player.UserId]
			task.wait(.1)
		until profile ~= nil
	else
		return profile
	end

You should check player:IsDescendantOf(Players)

repeat
    ...
until profile ~= nil or not player:IsDescendantOf(Players)

---

Template Ambiguity + Connections

I’d highly recommend removing the :SetTemplate() method and instead store the data template within this module itself or within a sub module. Main reason is 1 it requires you to remember to set a template, as well as storing said template in a different script. All this just makes it harder to find things, as well as adding a good amount of extra code that’s probably not needed.
(Also there is a potential connection problem, as if you call :SetTemplate() multiple times, there’s no condition check to make sure a template isn’t already active. Thus connecting another function to Players.PlayerAdded)

Instead just leave the data template at the top of the module and call have OnPlayerAdded be connected within the module

local dataProfiles = {}
local Profiles 	   = {}
local activeTemplate = {
    Money = 0,
    Gems = 0,
    Deaths = 0,
}

...

-- Add these somewhere functions after the onPlayerAdded / onPlayerRemoving functions
Players.PlayerAdded:Connect(onPlayerAdded)
Players.PlayerRemoving:Connect(onPlayerRemoving)
for _, player in pairs(Players:GetPlayers()) do
    onPlayerAdded(player)
end
-- Also these will only be connected the first time you require a module
-- So even if you require the data module from other scripts, this will only be connected once

If not, at least check if the data template already exists to prevent addition unnecessary connections

---

Potential unintended Behavior if Save Errors

Under the playerRemoving event, if the Save function errors, then the Profile won’t get removed nor will the signals gets disconnected.
This could cause unintended behavior with your script
Pcall save

local function onPlayerRemoving(player: Player)
	if typeof(player) ~= "Instance" then
		error(string.format("Player expected, got %s", typeof(player)))
	end
	
	Save(player) -- errors here
	Profiles[player.UserId] = nil -- not run VVVV
	
	-- disconnect all of the signals
	for _, signal in pairs(getmetatable(dataProfiles[player.UserId]).__events) do
		signal:DisconnectAll()
	end
	
	dataProfiles[player.UserId] = nil
end

    pcall(Save, player) -- if it errors, maybe print the error here
    Profiles[player.UserId] = nil -- still works

Code Design Feedback

general design stuff, more opinion based stuff

---

Type Checking

@BullfrogBait

If not, I’d suggest removing the excessive type checking. Especially with the PlayerAdded function.

Originally I thought you meant to remove the types from the function

I second this. For internal functions which are only called within the module, you definitely don’t need the additional if type(player) ~= "Instance" then error() end statement.

---

Types

You can use type in order to remove the need for comments
It’s cleaner and adds functionality to your code!

local Profiles 	   = {
	--[[
	
			["51739716"] = {
				Money = 100,
				Gems = 500,
				Deaths = 50
			}
	
	]]
}
--------------------------------------------------------------------------
-- can be changed to:
local Profiles : {
    [string] : 
    {
        Money:number, 
        Gems:number, 
        Deaths:number
    }
} = {}

-- alternatively
local Profiles = {} :: {
    [string]:{
        Money:number,
        Gems:number,
        Deaths:number
    }
}

--------------------------------------------------------------------------

-- If you define active template above profiles
-- you can use typeof(activeTemplate) 
-- Which also gives linting support and doesn't require updating (when you update the activeTemplate)

local activeTemplate = {
    Money = 0,
    Gems = 0,
    Deaths = 0
}
local Profiles : typeof(activeTemplate) = {}

-- You can condense these

---

Guard clauses and general code cleanup

Firstly:

String interpolation available for Studio

[Update] January 9, 2023 Hello everyone! Without further ado, we’re happy to release a beta feature for string interpolation! Please note, since this is a beta feature, it will only work in Studio. String interpolation is the new syntax that allows you to create a string literal with expressions inside of that string literal. You can read about it in the RFC for string interpolation. In short, it’s a safer and more ergonomic alternative over string.format. Here’s a quick example of a strin…

This is out of beta and can help remove string.format. (ignore that it doesn’t format on the devforum)

-- before
print(string.format("Saving %s's data is a success", player.Name))
-- after
print(`Saving {player.Name}'s data is a success`)

You can use guard clauses to reduce if statement chaining, which just cleaning up the code.

--- BEFORE
	mt.__newindex = function(t, k, v)
		-- if the indexed value is inside the template
		if activeTemplate[k] then
			-- if the value we're changing with is the same as the property
			if typeof(v) == typeof(activeTemplate[k]) then
				Profiles[player.UserId][k] = v
				-- if a signal exists for the property
				if mt.__events[k] then
					return mt.__events[k]:Fire(v)
				end
			else
				error(string.format("Unable to assign property %s. %s expected, got %s", k, typeof(activeTemplate[k]), typeof(v)))
			end
		else
			error(string.format("%s is not a valid member of activeTemplate", k))
		end
	end

---------------------------------------------------------------------------
--- AFTER
	mt.__newindex = function(t, k, v)
		if not activeTemplate[k] then
			error(`{k} is not a valid member of activeTemplate`) 
		elseif typeof(v) ~= typeof(activeTemplate[k]) then 
			error(`Unable to assign property {k}. {typeof(ActiveTemplate[k])} expected, got {typeof(v)}`)
		end
		
		Profiles[player.UserId][k] = v
		return mt.__events[k] and mt.__events[k]:Fire(v)
	end

-- Removed comments 
-- The return statement uses lua's version of ternary 

This can be reduced down through smart usage of loops

--- BEFORE
function Data:GetData(player: Player)
    if not player or not player:IsA("Player") then error(`{player} not a player`) end

	local profile = dataProfiles[player.UserId]
	
	if profile == nil then
		repeat
			profile = dataProfiles[player.UserId]
			task.wait(.1)
		until profile ~= nil or not player:IsDescendantOf(Players)
	else
		return profile
	end
	
	return profile
end

---------------------------------------------------------------------------
--- AFTER
function Data:GetData(player: Player)
    if not player or not player:IsA("Player") then error(`{player} not a player`) end

	repeat
		local profile = dataProfiles[player.UserId]
		if profile then return profile end
		task.wait(.1)
	until not player:IsDescendantOf(Players)
end

Final tidbit, even though this isn’t lua, there’s some good information about comments in this video Don't Write Comments - YouTube
Would recommend checking it out (along with that channel), since it has a lot of good information about writing good code.

But at the end of the day, your code is your code, feel free to code however you choose