What can i improve in this data saving module?

[0786ideal (ideal)]

Hey, i wanted to ask what can i improve in this simple module, i made it to make data store more sorted by sections like tools, inventory ect.

if there is anything to improve, please tell me that, thx for help

and here is a code, there is main script that calls

local Functions = {}

local DataStoreService = game:GetService("DataStoreService")
local DS1 = DataStoreService:GetDataStore("DS1")
local Inventory = game:GetService("ReplicatedStorage").Inventories

local ValueTypes = {
	["boolean"] = "BoolValue",
	["string"] = "StringValue",
	["number"] = "NumberValue",
	["integer"] = "IntValue"
}
----------------------------------------------------------------------------------
----------------------------------------------------------------------------------
--[[Creates new value under given folder]]
local function CreateNewValue(name: string, value: any, parent: Instance)
	local newObject = Instance.new(ValueTypes[typeof(value)])
	newObject.Name = name
	newObject.Value = value
	newObject.Parent = parent
end
--[[Creates folder with player data in Replicated Storage]]
local function LoadData(player: Player, data: {})
	local PlayerInventory = Inventory:FindFirstChild(player.UserId)
	for name, value in data do
		if typeof(value) == "table" then
			local newFolder = Instance.new("Folder")
			newFolder.Name = name
			newFolder.Parent = PlayerInventory
			for objectName, newValue in value do
				CreateNewValue(objectName,newValue,newFolder)
			end
		else
			CreateNewValue(name,value,PlayerInventory)
		end
	end
end
--[[Serializes inventory into table]]
local function SerializeData(PlayerInventory: Folder) : {}
	local data = {}
	for _, object in PlayerInventory:GetChildren() do
		if object:IsA("Folder") then
			data[object.Name] = {}
			
			for _, value in object:GetChildren() do
				data[object.Name][value.Name] = value.Value
			end
			
		else
			data[object.Name] = object.Value
		end
	end
	
	return data
end
--[[Creates new inventory when player joins for the first time]]
function Functions.newInventory(player: Player)
	local newInventory = Instance.new("Folder")
	newInventory.Name = player.UserId
	newInventory.Parent = Inventory
	
	local PlayerInventory = Instance.new("Folder")
	PlayerInventory.Name = "Inventory"
	PlayerInventory.Parent = newInventory
	
	local Id = Instance.new("IntValue")
	Id.Name = "Id"
	Id.Value = 0
	Id.Parent = newInventory
end
--[[loads player's data]]
function Functions:Load(player: Player)
	local state = true
	local data
	local key = player.UserId
	
	local succes,err = pcall(function()
		data = DS1:GetAsync(key)
	end)
	
	if succes and data then
		local PlayerInventory = Instance.new("Folder")
		PlayerInventory.Name = tostring(player.UserId)
		PlayerInventory.Parent = Inventory
		
		LoadData(player,data)
	elseif data == nil then
		state = false
	else
		warn("Error while loading data... ", err)
	end
	
	return state
end

function Functions:Save(player: Player, autoSave: boolean)
	local PlayerInventory = Inventory:FindFirstChild(player.UserId)
	local DataToSave = SerializeData(PlayerInventory)
	local key = player.UserId
	
	if not autoSave then PlayerInventory:Destroy() end
	
	local succes, err = pcall(function()
		DS1:UpdateAsync(key, function(oldData)
			if not oldData then
				DataToSave.Id = 0
				return DataToSave
			elseif oldData.Id == DataToSave.Id then
				DataToSave.Id += 1
				return DataToSave
			else
				return nil
			end
		end)
	end)
	if succes then
		return true
	else
		warn(err)
		return false
	end
	
end

return Functions

[KhalsUK]

It seems okay, and good job on a few of the architectural decisions!

Few questions:

• How come you swap between using : and . for defining some functions vs others? As far as I can see you don’t use self in the functions defined with a : which is the typical usecase
• How come you delete the PlayerInventory instance specifically when the save is not an autosave? I’m guessing this is only called with autoSave as false or nil when the player leaves - but if so, why not just let it get destroyed with the Player instance?
• Is there a reason for using a custom implementation as opposed to something like ProfileService (and possibly ReplicaService for propogating your state structrue) (nothing wrong with it, I think it’s cool you’re trying it out yourself, just a general ‘how come?’)

[0786ideal (ideal)]

Thanks for help, a little bit late, i made 2 more versions of it, and scraped code two times, i decided to make second module to serialize data that i need for my specific system (non stackables that have to be compressed) also i decided to remove newInventory function, instead i added default Inventory which contains important stuff

I wanted to ask if using local functions inside module is OK for cleaner code, idk if only i think it looks bad, thx for feedback anyways

[KhalsUK]

Ah ok apologies about that - I clicked onto the topic and yours showed as the top one for some reason haha

Using local functions inside a module is completely fine - I typically refer to these as either ‘private’ or ‘helper’ functions - ‘private’ because the scripts requiring them cant use them, ‘helper’ because they help the public functions - the ones that can be accessed externally - to perform certain repeated routines to avoid repeating yourself in multiple public functions

Hope this helps

[0786ideal (ideal)]

Thanks for help, i’ll surely use some of your tips :}