Datastore Code Review

Help and Feedback Code Review
#1

How is my datastore code?


local ServerStorage = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game.Players
local MarketPlaceService = game:GetService("MarketplaceService")
local DataStoreService = game:GetService("DataStoreService")
local StatsDS = DataStoreService:GetGlobalDataStore("StatsDatastore")
local InventoryDS = DataStoreService:GetGlobalDataStore("InventoryDataStore")
local MainDS = DataStoreService:GetDataStore("MainDataStore")
local PlayerCount = #game.Players:GetChildren()
local HttpService  = game:GetService("HttpService")


Players.PlayerAdded:Connect(function(player)
	local Stats = Instance.new("Folder")
	Stats.Name = "PlayerStats"
	
	local Inventory = Instance.new("Folder")
	Inventory.Name = "Inventory"
	
	local Coins = Instance.new("NumberValue")
	Coins.Name = "Coins"
	
	local Hotbar = Instance.new("StringValue")
	Hotbar.Name = "Hotbar"
	
	Stats.Parent = player
	Inventory.Parent = player
	Coins.Parent = Stats
	Hotbar.Parent = Inventory
	
	

	local main = nil

	local success,errormsg = pcall(function()
	  main = MainDS:GetAsync(player.UserId)
	end)
	
	if success and main ~= nil then
		local p = HttpService:JSONDecode(main).playerstats
		local i = HttpService:JSONDecode(main).inventory
		for i,value in pairs(player.PlayerStats:GetChildren()) do
			if value:IsA("ValueBase") then
				for i=1,#p do
					if p[i][1] == value.Name then
						value.Value = tonumber(p[i][2])
					end
				end
			end
		end
	end
	
end)

Players.PlayerRemoving:Connect(function(player)
	wait()
	local datasave = {["playerstats"]={},["inventory"]={}}
	
	for i,value in pairs(player.PlayerStats:GetChildren()) do
		if value:IsA("ValueBase") then
			local ps = datasave.playerstats
			table.insert(ps,#ps+1,{value.Name,value.Value})
		end
	end
	
	for i,block in pairs(player.Inventory:GetChildren()) do
		if block:IsA("NumberValue") then
			local I = datasave.inventory
			table.insert(I,#I+1,{block.name,block.Value})
		end
	end
	
	local success,m = pcall(function()
           MainDS:SetAsync(player.UserId,HttpService:JSONEncode(datasave))
	end)
	
	if m then
		warn(m)
	else
		print(HttpService:JSONEncode(datasave))
		print(player.Name.."'s Data Was Saved")
	end
end)
3 Likes
#2

As with every other case of a DataStore code review, use UpdateAsync instead of SetAsync if you don’t want the most annoying possible issues with data loss.

1 Like
#3

So would it be this?

MainDS:UpdateAsync(player.UserId,HttpService:JSONEncode(datasave))

Edit: nvm I got it.

#4

Why are you JSON encoding? Doesn’t Roblox already do that for you?

#5

Wait, so when you save data, it Encodes it?

1 Like
#6

It’s a long time since I’ve used datastore service, but if I recall correctly it does that for you automatically.

#7

oh, so I can just save the table? What would happen if you encode a json two times?

1 Like
#8

I’m currently on phone so I can’t test my theory but you could try it yourself, I’m very confident that’s the case.

Encoding it several times is not recommended either, as it just increases the size.

1 Like
#9

The obly reason anyone would want to encode data to JSON when saving to datastores is for custim compression.

#10

I’m not sure how exactly you’re using custom compression when encoding the data to JSON after doing your logic.

When I think of custom compression I “super-compress” the data with absolutely no redundant information in the table. Redundant data being any data that’s in the table that could be declared in a script instead.

If you want me to explain the logic further please notify me. Basically, instead of having {name=“john doe”}, I am doing “john doe” and that’s it. It’s just a string, then I can separate the next values with a comma. Then, you can use a compression library like this one: Text compression

#11

I actually use my own module of a custom JSON like data format that removes the redundant parts of JSON (the size is only decreased by 5%-10% , but that’s still something), then run it through a simple LZW function. That would also work for JSON.

Basically, this:

{a=1.1234,b=CFrame.new(1,0,0)*CFrame.Angles(20,0,0),c={1,2,3}}

Is converted to this:

a#1.12#b(cf,1,0,0,20)c{#1##2##3#}

1 Like
#12

There are a couple of issues I’ve noticed with your code. Note that I’m going in with the assumption that you’ve laid everything bare in this script and left nothing out.

If you do have other processes running but didn’t include them, it’d be helpful to cut out the variables linked to those other processes as well so it doesn’t look as though you have needless declarations. For example, MarketplaceService is declared but never used as far as code reviewers are concerned.

I. Excess of variables.

The top of your script, which holds your variable declarations, very simply put, is ugly to look at. In the cases where you don’t use any of those variables or need them declared immediately, get rid of them. Here’s a list of what I’m looking at:

  • ServerStorage: Not used anywhere in your code.

  • ReplicatedStorage: Ditto.

  • MarketplaceService: Ditto.

  • HttpService: Use of JSON is unnecessary. Roblox does this under the hood and all you achieve by encoding is inflating your DataStore character count. Either use a custom format or save raw. If you do this for the sake of compression, look into other compression strategies.

Also worth noting that there’s maximum of 200 local upvalues. You probably won’t ever hit that limit but just keep that in the back of your mind.

II. Inconsistency, redundancy and poor organisation of variables.

One very clear example is that you index the Players service via dot syntax and go on later to write the full service path again but to use GetChildren (which is canonically incorrect, you should be using GetPlayers). If you set up a variable, then use the path.

As far as poor organisation of variables goes, you just have a mass of declarations at the top. Try putting them into a clearer format and separating them with new lines. I personally put service declarations at the top, followed by constants and variables.

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

-- Including "Datastore" in a DataStore namespace is unnecessary!
-- If you're working with a DS namespace, the fact that it's a DS is implied.
local MainStore = DataStoreService:GetDataStore("Main") -- See III.
local StatsStore = DataStoreService:GetDataStore("Stats") -- See IV.
local InventoryStore = DataStoreService:GetDataStore("Inventory") -- Ditto.

III. Potential excessive usage of DataStores.

In the code sample you’ve provided, you have three DataStores set up per player. This means that per player, you use three requests of any budget for any task. You need to keep DataStore limitations in mind. Even worse, if you decide to change to DataStore2 in the future, you increase your request amount to n*2 requests, ignoring combined stores.

You don’t need three DataStores! I don’t even see you using the other two! When working with DataStores, you should always try aiming to consolidate your stores as much as possible with the use of dictionaries. Create partitions in a larger table and save there.

{MainStore
    {Stats
        A=1
    }
    {Inventory
        B=1
    }
    (other)
    C=1
}

IV. Use of GlobalDataStore for player data.

You’re using the wrong function in this case. GetGlobalDataStore returns the default DataStore and should only be used if you need game-wide configuration access. GetGlobalDataStore does not accept a name parameter, so the string you pass to it is thrown away.

Simply put: you may experience overwrites or data loss by using GetGlobalDataStore and you shouldn’t be using it for player data in the first place. You must use GetDataStore for named DataStores and for partitioning.

V. Parenting children in the wrong order.

In any typical scenario, you should always set parents from the bottom up rather than the top down. This almost guarantees that the descendants will be available when the ancestor replicates. It’s always good practice to have the descendants available when the ancestors are!

Hotbar.Parent = Inventory
Coins.Parent = Stats
Inventory.Parent = player
Stats.Parent = player

On that note though, a small practice related thing: Coins probably belongs in inventory and you should aim to include all these stats either in some kind of single folder hierarchy, as such:

ex1

Player
    Data
        Inventory
            Hotbar
        Stats
            Coins

Player
    Data
        Hotbar
        Coins

VI. Pcall wrapping.

Okay, I really need to write that pcall tutorial.

You should try to stay away from using upvalues to get pcall values. Pcall is a callback, therefore you can return the results of a pcall or directly wrap the function call. I highly suggest doing the latter. Here are the examples of returning and direct wrapping:

local success, result = pcall(function ()
    return MainDS:GetAsync(player.UserId)
end)

local success, result = pcall(MainDS.GetAsync, MainDS, player.UserId)

VII. Iterating over an array.

If you’re iterating over an array, the keys of it are guaranteed to be in numerical order and not arbitrary. In this case, please use ipairs and not pairs. ipairs runs faster than pairs, especially with LuaU and ipairs was designed specifically with arrays in mind while pairs for dictionaries.

This applies for all instances of your use of pairs while iterating through arrays (GetChildren generates and returns an array). Please go back and correct all of those!

VIII. Yielding at the start of PlayerRemoving.

Simply put: don’t. You don’t need to. Remove the wait.

Here’s a loosely (or possibly directly) related thread that you’d should read. The short of it is that wait has no real place in production code.

That’s all! Good luck developing out there.

2 Likes