What do you guys think of my data module?

Help and Feedback Code Review
#1

This is my stats module that handles and save stats. It works fine and has customizable settings. I am wondering what I should do to improve this. Thanks

local StatsManager  = {}
local Settings = {}

Settings.TriesAllowed = 3
Settings.DataKey = "BetaKey"
Settings.StudioSave = false
Settings.DefaultTable = {Cash = 0}

local MainStore = game:GetService("DataStoreService"):GetDataStore(Settings.DataKey)

local StatReplicator = require(script.Parent.StatReplicator)

function StatsManager:SetupPlayerData(player)
	local data 
	local tries = 0
	local success
	repeat
		tries = tries + 1
		success = pcall(function()
			data = MainStore:GetAsync(player.UserId) 
		end)
		if not success then wait(1) end
	until tries == 3 or success 
	
	if not success then
		warn("error loading data for "..player.Name)
		return 
	end
	
	if data then
		StatsManager[player.UserId] = data
	else
		StatsManager[player.UserId] = Settings.DefaultTable
	end
end

function StatsManager:IncrementStat(player, statName, incrementValue)
	if StatsManager[player.UserId][statName] ~= nil then
		StatsManager[player.UserId][statName] = StatsManager[player.UserId][statName] + incrementValue
	else
		warn(statName.." does not exist for "..player.Name)
	end
end

function StatsManager:SetStat(player, statName, newValue)
	if StatsManager[player.UserId][statName] ~= nil then
		StatsManager[player.UserId][statName] = newValue
	else
		warn(statName.." does not exist for "..player.Name)
	end
end

function StatsManager:SaveStats(player)
	local tries = 0
	
	local success
	
	repeat
		tries = tries + 1
		success = pcall(function()
			MainStore:SetAsync(player.UserId, StatsManager[player.UserId])
		end)
		if not success then wait(1) end
	until tries == 3 or success 
	
	if not success then
		warn("error saving data")
	end
end

function StatsManager:GetStat(player, statName)
	if StatsManager[player.UserId][statName] ~= nil then
		return StatsManager[player.UserId][statName] 
	else
		warn(statName.." does not exist for "..player.Name)
	end
end

game.Players.PlayerRemoving:Connect(function(player)
	if not Settings.StudioSave then return end
	StatsManager:SaveStats(player)
end)

game:BindToClose(function()
	if game:GetService("RunService"):IsStudio() == false then
		for _,player in ipairs(game.Players:GetPlayers()) do
			wait(1)
			StatsManager:SaveStats(player)
		end
	else
		if not Settings.StudioSave then return end
		wait(2)
	end
end)
	 
return StatsManager
2 Likes
#2

There is a bug in the code where all new players share the same data table, because you instantiate it only once. As such, two people just starting will have both their data changed when either does anything.

Also, your functions should be defined with function x.y() syntax, not function x:y() syntax, as the latter is used for objects.

#3

Can you elaborate on that? I dont see the glitch as I call setup player data for each player on a player added event. Also, what do you mean when you say x:y is for obejcts? I thought the only difference was x:y passes self as a paramter automatically?

#4

Yes. The parameter is specific to objects. You are calling it directly, not as an object.

Also, the bug is at StatsManager[player.UserId] = Settings.DefaultTable where you just give it the reference of the same table. If you have two new players and increment the coins of the first player, it will do so for both.

#5

You’ll need to do a deep clone of the table. A simple example could be,

function cloneTable( t )
    local newTab = {}
    for i, v in pairs( t ) do
        newTab[ i ] = type( v ) == 'table' and cloneTable( v ) or v
    end
    return newTab
end

--...
StatsManager[ player.UserId ] = cloneTable( Settings.DefaultTable )
--...
#6

Thanks guys for helping me out! :grin:

#7 
Settings.TriesAllowed = 3
Settings.DataKey = "BetaKey"
Settings.StudioSave = false
Settings.DefaultTable = {Cash = 0}

Instead of creating them, why don’t you just first add them to the table manually instead of the scripting having to create it?

local Settings = {
  TriesAllowed = 3
  StudioSave = false
  DefaultTable = {Cash = 0}
  DataKey = "BetaKey"
}

Since I’m not only going for efficiency, but also for code structure and readability.

function StatsManager:SetupPlayerData(player)
	local data 
	local tries = 0
	local success
	repeat
		tries = tries + 1
		success = pcall(function()
			data = MainStore:GetAsync(player.UserId) 
		end)
		if not success then wait(1) end
	until tries == 3 or success 
	
	if not success then
		warn("error loading data for "..player.Name)
		return 
	end
	
	if data then
		StatsManager[player.UserId] = data
	else
		StatsManager[player.UserId] = Settings.DefaultTable
	end
end

Why are you increasing tries every repeat? You only should increase it after the pcall fails.

	if not success then
		warn("error loading data for "..player.Name)
		return 
	end

You never print out the error (which you should) which the pcall returns after it wasn’t successful. Same goes for your SaveStats function.

game:BindToClose(function()
	if game:GetService("RunService"):IsStudio() == false then
		for _,player in ipairs(game.Players:GetPlayers()) do
			wait(1)
			StatsManager:SaveStats(player)
		end
	else
		if not Settings.StudioSave then return end
		wait(2)
	end
end)

Instead of explicitly checking if it’s false, you can just do game:GetService("RunService"):IsStudio() since it returns a boolean.

Also, why are you using : when you aren’t even using self in the first place? You should use ..

Here’s how I would write your code:

local StatsManager  = {}

local Settings = {
	MAX_TRIES_ALLOWED = 3;
	DATA_KEY = "BetaKey";
	Save_In_Studio = false;
	DEFAULT_DATA_TABLE = {Cash = 0};
}

local DataStoreService = game:GetService("DataStoreService")
local RunService = game:GetService("RunService")
local Players = game:GetService("Players")

DataStoreService:GetDataStore(Settings.DataKey)

local StatReplicator = require(script.Parent.StatReplicator)

function StatsManager.SetupPlayerData(player)
	
	if not player:IsA("Instance") or not player:IsDescendantOf(Players) then return end
	
	local data 
	local tries = 0
	local success

	repeat wait()

		local success, error = pcall(function()
			data = DataStoreService:GetAsync(player.UserId) 
		end)

		if not success then wait(1) tries += 1  end
		
   	until tries == 3 or success 

	if not success then
		warn("Error loading data for ".. player.Name .. " Error: " .. error)
		return 
	end

	if data then
		StatsManager[player.UserId] = data
	else
		StatsManager[player.UserId] = Settings.DefaultTable
	end
end

function StatsManager.IncrementStat(player, statName, incrementValue)
	
	if not player:IsA("Instance") or not player:IsDescendantOf(Players) then return end
	
	if typeof(incrementValue) == "number" and StatsManager[player.UserId][statName] then
		StatsManager[player.UserId][statName] = StatsManager[player.UserId][statName] + incrementValue
	else
		warn(statName .." does not exist for ".. player.Name .. " or incrementValue was not a number")
	end
end

function StatsManager.SetStat(player, statName, newValue)
	if not player:IsA("Instance") or not player:IsDescendantOf(Players) then return end
	
	if StatsManager[player.UserId][statName] then
		StatsManager[player.UserId][statName] = newValue
	else
		warn(statName.." does not exist for ".. player.Name)
	end
end

function StatsManager.SaveStats(player)
	if not Settings.StudioSave then return end
	
	local tries = 0

	local success

	repeat
		local success, error = pcall(DataStoreService.SetAsync, DataStoreService, player.UserId, StatsManager[player.UserId])
		
		if not success then wait(1) tries += 1 end
		
	   until tries == Settings.MAX_TRIES_ALLOWED or success 

	if not success then
		warn("error saving data")
	end
end

function StatsManager.GetStat(player, statName)
	if not player:IsA("Instance") or not player:IsDescendantOf(Players) then return end
	
	if StatsManager[player.UserId][statName] ~= nil then
		return StatsManager[player.UserId][statName] 
	else
		warn(statName.." does not exist for "..player.Name)
	end
end

Players.PlayerRemoving:Connect(StatsManager.SaveStats)

game:BindToClose(function()
	if RunService:IsStudio() and Settings.Save_In_Studio then
		for _,player in ipairs(game.Players:GetPlayers()) do
			coroutine.wrap(StatsManager.SaveStats)(player)
		end
	else
		return 
	end
end)

return StatsManager
#8

Thanks for your advice, after careful consideration though I am switching to profiileservice as it is really well made

1 Like