Is there any other way to improve my DataStore more?

Help and Feedback Code Review
#1

Is there any other way to improve and make my code any more simple than it is now?

Script

local sp = script.Parent;
local Main = sp.Parent;
local Modules = Main:WaitForChild("Module");
local DataStoreSettings = require(Modules:WaitForChild("Settings"));
local StatsData = game:GetService("DataStoreService"):GetDataStore(DataStoreSettings.StatsDataStore);
local DataStoreModule = require(Modules:WaitForChild("DataStore"));

local Stats = {
	{"Coins","NumberValue",0};
};

function PlayerAdded(Player)
	local leaderstats = Instance.new("Folder");
	leaderstats.Name = "leaderstats";
	leaderstats.Parent = Player;
	
	for i,v in next,Stats do
		local Value = Instance.new(v[2]);
		Value.Name = v[1];
		Value.Value = v[3];
		Value.Parent = leaderstats;
	end
	
	DataStoreModule.LoadData(Player,leaderstats,StatsData);
end

function PlayerLeft(Player)
	pcall(function()
		StatsData:SetAsync(Player.UserId,DataStoreModule.SaveData(Player.leaderstats));
	end)
end

function Shutdown()
	local Players = {};
	
	for i,v in next,game.Players:GetChildren() do
		PlayerLeft(v);
		table.insert(Players,v);
	end
	
	for i,v in next,Players do
		game:GetService("TeleportService"):Teleport(game.PlaceId,v);
	end
end

game.Players.PlayerRemoving:Connect(PlayerLeft);
game.Players.PlayerAdded:Connect(PlayerAdded);

game:BindToClose(function()
	Shutdown();
	while wait() do
		-- keep the server run as longest possible
	end
	-- ahh we are all gonna die
end)

DataStoreModule

This is the script that handles the Data Load and Save.

local function LoadData(Player,Inventory,Data)
	local UserData;
	pcall(function()
		UserData = Data:GetAsync(Player.UserId);
	end)
	if (UserData) then
		for i,v in next,UserData do
			--[[
				v = {name = v.Name;value = v.Value};
			--]]
			if (Inventory:FindFirstChild(v.name)) then
				Inventory[v.name].Value = v.value;
			else
				warn(v.name.." is not a valid member of "..Inventory.Name)
			end
		end
	end
end

local function SaveData(Inventory)
	local DataSave = {};
	for i,v in next,Inventory:GetChildren() do
		local TempData = {
			name = v.Name;
			value = v.Value;
		};
		-- TempData = {name = v.Name;value = v.Value};
		table.insert(DataSave,TempData);
	end

	return DataSave;
end

local Module = {};

Module.LoadData = LoadData;
Module.SaveData = SaveData;

return Module;

Data Key

return {
	StatsDataStore = "randkey";
};

Looks like

image

Why is my datastore not saving?
#2

You should try not to make use of values as much.

#3

Just a note here, on your Shutdown function…

https://developer.roblox.com/api-reference/function/Player/Kick

function Shutdown()
        -- Save their data first off like you have.
        for i,v in next,game.Players:GetChildren() do
		PlayerLeft(v);
	end
        -- Then rather than teleporting them back to the place. Try this..
        for _,player in next,game.Players:GetPlayers() do
               player:Kick("Reason here")
        end
        -- I mean I understand what you are trying to do, but just use :Kick() -> wiki link above this code block.
        -- It just gives the user a reason and simple "reconnect" button when a new server comes up.
        -- Its just more efficient. :slight_smile: 
end
1 Like
#4

No need to call PlayerLeft. When the player leaves, it’d be automatically fired :slight_smile:

1 Like
#5

I just wanted to edit one part of the code lol. Primarily a better way to kick a player from the game.

1 Like
#6

Ok everyone is talking about this event and code inside in the event my question was how is the script built and the style.

game:BindToClose(function()

end)
#7

Your LoadData function is dangerous. You pcall the GetAsync (good), but if it fails it just sets the player’s data to empty, and your SaveData function has no knowledge of whether or not the data its saving is their original data.

If data stores go down and people play your game, they will lose all their data.

Why do you have the loop? You should instead just make a BindableEvent and yield on it until all shutdown operations/saving finishes.

4 Likes
#8

Ohhhhh, that is something that I have not thought of.

#9

For shutting down a game I don’t recommend kicking the players, but teleport them to a reserved server (the same server, but really a private server) like this script that Merely did.

Soft Shutdown Script

2 Likes
#10

Well, we keep the most of the players that were in the game.
So yeah, that is why I made so it teleports them to the same game.

but yes it kicks them again xD

#11

But if you teleport them to the same game you could get in a loop of where you teleport the player in a server that’s trying to shutdown that script teleports the player in a reserved server then after like 10 seconds teleports them back to the main servers.

#12

Your usage of SetAsync is uncomfortable. In addition to what Kampf mentioned, I personally would not use SetAsync unless there’s a reason I need to force data to be a certain way. I’d prefer using UpdateAsync to minimise issues while updating data - it does exist for a reason, as the name states, UpdateAsync. I find that Set is more for setting when it’s needed (i.e. PlayerData doesn’t exist).

Your shutdown function isn’t going to do much. All you’re doing is teleporting players back to the game. This doesn’t give the game a chance to shutdown old servers, especially since you’re hooking functions to OnClose (which allows a maximum of 30 seconds to finish operations binded to it before finally closing the server) - servers can be kept alive and negate the purpose of this function.

@Shadrz Why? If you’re going to suggest something, elaborate on your idea. Don’t just tell someone not to do something and forego reasoning.

1 Like
#13

I see this all the time when people use pcall. There is no need to wrap this in a function as pcall takes a function followed by any arguments to pass to that function.

pcall(function()
	StatsData:SetAsync(Player.UserId,DataStoreModule.SaveData(Player.leaderstats));
end)

You can use

--  you need to pass the table as : is used
pcall(StatsData.SetAsync, StatsData, Player.UserId, DataStoreModule.SaveData(Player.leaderstats))
1 Like
#14

What about this?

pcall(function()
StatsData1:SetAsync(Player.UserId,DataStoreModule.SaveData(Player.leaderstats));
StatsData2:SetAsync(Player.UserId,DataStoreModule.SaveData(Player.leaderstats1));
end)
#15

You would want to use two pcalls as you do not want to duplicate the request if one fails.

2 Likes
#16

So what I’m trying to say is that he should use metatables, I’m not great at them but you should definitely put them in a module.

#17

Metatables are not going to help in this use case. Metatables are not an alternative to ValueObjects, they’re intended to run metamethods on tables for certain cases, including the following cases:

  • If a member of the table is indexed or getting an extended index
  • Deciding what is to be done when a new value is created
  • Determining what should happen if a table variable is called like a function
  • etc.

See the metamethods Developer Hub article for more information on metamethods (since that’s the only real part of metatables you need to know, metatables are as simple as setmetatable(table, metatable)).

A proper alternative to ValueObjects is ModuleScripts, which OP is using aside from the actual registration of data which is loaded out across ValueObjects. Depending on OP’s use case and preferences, this advice could hold no value to them.

2 Likes
#18

Oh ok, that makes sense.

#19

My recommendation is what I usually do: Cache the data in a table. When I write Data Store modules I try to make as little use of “the Async’s” as possible.

#20

So how can i fix that?