Data Manager script

Help and Feedback Code Review
#1

I need some thoughts/opinions on this script I’m making for a simulator game. I haven’t commented that much in this script, so feel free to ask what a certain function/piece of code does.

It would help a lot if you gave me some feedback, especially if its to organize my code even further :grin:

(This uses ProfileService btw)

--/ Services
local RunService = game:GetService("RunService")
local ServerStorage = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")

--/ Modules
local ProfileService = require(ServerStorage.Modules.ProfileService)

--/ Instances
local EventsFolder = ReplicatedStorage:WaitForChild("RemoteEvents")
local AddClicks = EventsFolder:WaitForChild("AddClick")
local UpdateMultipliers = EventsFolder:WaitForChild("UpdateMultipliers")

local Pets = ServerStorage:WaitForChild("Pets")

--/ Varibles
local profile_template = {
	Coins = 0;
	Dimonds = 25;
	Rebirths = 0;
	Inventory = {"Example"}
}
local datastore_name = "PlayerData_346"

local ProfileStore = ProfileService.GetProfileStore(
	datastore_name,
	profile_template
)

local Profiles = {}

--/ Functions
local function onPlayerAdded(Player)
	local profile = ProfileStore:LoadProfileAsync(
		"Player_"..Player.UserId,
		"ForceLoad"
	)
	
	if profile ~= nil then
		profile:Reconcile()
		profile:ListenToRelease(function()
			Profiles[Player] = nil
			Player:Kick()
		end)
		if Player:IsDescendantOf(Players) == true then
			Profiles[Player] = profile
		else
			profile:Release()
		end
	else
		Player:Kick() 
	end
	
	local leaderstats = Instance.new("Folder", Player)
	leaderstats.Name = "leaderstats"
	
	local currency = Instance.new("IntValue", leaderstats)
	local higherCurrency = Instance.new("IntValue", leaderstats)
	local rebirths = Instance.new("IntValue", leaderstats)
	
	currency.Name = "Coins"
	higherCurrency.Name = "Dimonds"
	rebirths.Name = "Rebirths"
	
	currency.Value = profile.Data.Coins
	higherCurrency.Value = profile.Data.Dimonds
	rebirths.Value = profile.Data.Rebirths
	
	local petsFolder = Instance.new("Folder", Player)
	petsFolder.Name = "Pets"
	
	
	for pets, pet in ipairs(profile.Data.Inventory) do -- profile.Data.Inventory has an example pet in it by default
		ServerStorage.Pets:WaitForChild(pet):Clone().Parent = Player.Pets
	end
end

local function onPlayerRemoved(Player)
	local profile = Profiles[Player]
	
	if profile ~= nil then
		profile:Release()
		print(Player.Name .. "'s profile has been released!")
	end
end

local function addClicks(Player) -- For adding cash to the player
	local profile = Profiles[Player]
	local multiplier = 0
	
	for index, value in ipairs(profile.Data.Inventory) do
		multiplier += Pets:WaitForChild(value).Multiplier.Value
	end
	
	if multiplier == nil then
		multiplier = 1
	end
	
	Player.leaderstats.Coins.Value += 1 * multiplier
	profile.Data.Coins += 1 * multiplier
end

function addPets(Player, PetToAdd)
	local profile = Profiles[Player]
	local pet = ServerStorage.Pets[PetToAdd]:Clone()
	
	pet.Parent = Player.Pets
	table.insert(profile.Data.Inventory, PetToAdd)
end

--/ Code

for _, player in ipairs(Players:GetPlayers()) do
	coroutine.wrap(onPlayerAdded)(player)
end

--/ Function Callers
Players.PlayerAdded:Connect(onPlayerAdded)
Players.PlayerRemoving:Connect(onPlayerRemoved)
AddClicks.OnServerEvent:Connect(addClicks)
3 Likes
#2

There’s a bug in your code:

if multiplier == nil then

should probably be

if multiplier == 0 then

In what situations can that happen? Is it a catastrophic failure that means ProfileService has a bug? In that case you should probably throw an error so you know something is wrong. Either way, this is exactly the kind of situation where a comment can help - it’s obvious what the code does but not why it does it. Same with this block:

        if Player:IsDescendantOf(Players) == true then
			Profiles[Player] = profile
		else
			profile:Release()
		end

I wouldn’t change a lot regarding comments in general tho, you’re using them really well.

Your DataManager script probably shouldn’t be concerned with pets or leaderstats. I’d have separate PetManager and LeaderstatManager scripts listen to a OnPlayerDataReady BindableEvent (unless ProfileService already has event you can listen to) to setup all pet and leaderstat related logic when a given player’s data is ready.

This would make your separation of concerns better both in your entire code base but also for the specific onPlayerAdded function. Separation of concerns - Wikipedia

If you must have the DataManager deal with pets and leaderstats, separate it into separate functions or at least add comments to visually separate the concerns.

for pets, pet in ipairs(profile.Data.Inventory) do

I’d rename pet to petName to better communicate that this is a string, and is the name of a pet rather than a pet object. The pets variable is unused and should be replaced with _, and even if it was used it’d be a really confusing variable name.

addClicks(Player) seems to only add one click, so I’d rename it to addClick(Player). Same with addPets. Plural/singular can say a lot about what a variable holds, if it’s an object or a collection of objects. Also the addPets function seems to be unused so it should be removed if you don’t need it.

for _, player in ipairs(Players:GetPlayers()) do
	coroutine.wrap(onPlayerAdded)(player)
end
Players.PlayerAdded:Connect(onPlayerAdded)

I’m not sure this is necessary, if it is tho please correct me. I believe any scripts in ServerScriptService will always run before the first PlayerAdded event, and that all objects will be set up on the server before scripts run so you don’t need WaitForChild, so unless some of the previous code can yield then it shouldn’t be necessary to check for existing players. If require(ServerStorage.Modules.ProfileService) yields then there’s not much you can do about it in a cleaner way tho.

Other than that your code is really good, with good and consistent style and good organization leading to very readable code, except for the separation of concerns issue I mentioned earlier. It’s not a big deal for this little code, but as your project grows in size and complexity it can really make it a lot easier to understand what’s going on if everything is handled in the most obvious place possible.

To improve you could work on making an entire game, and see how well your organization works on a larger scale.

Hope this was helpful :slight_smile: Let me know if something is unclear or if you have any questions in general :+1:

1 Like
#3

In what situations can that happen? Is it a catastrophic failure that means ProfileService has a bug? In that case you should probably throw an error so you know something is wrong. Either way, this is exactly the kind of situation where a comment can help - it’s obvious what the code does but not why it does it. Same with this block:

ProfileService will return nil from LoadDataAsync when several edge cases are meeted with. Therefore that if statement is necessary.

I’m not sure this is necessary, if it is tho please correct me. I believe any scripts in ServerScriptService will always run before the first PlayerAdded event, and that all objects will be set up on the server before scripts run so you don’t need WaitForChild, so unless some of the previous code can yield then it shouldn’t be necessary to check for existing players. If require(ServerStorage.Modules.ProfileService) yields then there’s not much you can do about it in a cleaner way tho.

They are necessary. This is a problem since usually some scripts run late and by the time they run, a player joins. This is a problem due to the fact that scripts are deferred, (even if Workspace.SignalBehaviour isn’t set to deferred). See Deferred Events.

You can actually see the behaviour of scripts running late by a simple test:

print(#game.Players:GetPlayers())

If this prints 1, then it means the script just after a player join. This happens usually occasionally.

Ran some tests, and these days it happens more often than occasionally:

image

3 Likes
#4

Helped a lot, thanks! [filler]

1 Like