Messiest script ever

Help and Feedback Scripting Support
#1

How terrible is this datastore and any advice on making it better/cleaner ?

local dataStoreService = game:GetService("DataStoreService")
local leaderstatsDataStore = dataStoreService:GetGlobalDataStore("leaderstats")
local leaderboardDataStore = dataStoreService:GetGlobalDataStore("leaderboard")
local data = dataStoreService:GetDataStore("Stats")

game.Players.PlayerAdded:Connect(function(Player)
     
    local leaderstats = Instance.new("Folder", Player)
	leaderstats.Name = "leaderstats"

	local leaderboard = Instance.new("Folder", Player)
	leaderboard.Name = "leaderboard"

	local Level = Instance.new("NumberValue", leaderstats)
	Level.Name = "Level"
	Level.Value = 1

	local Exp = Instance.new("NumberValue", leaderboard)
	Exp.Name = "Exp"
	Exp.Value = 0

	local RequiredEXP = Instance.new("NumberValue", Player)
	RequiredEXP.Name = "RequiredExp"
	RequiredEXP.Value = Level.Value * 100

	local Wins = Instance.new("NumberValue", leaderstats)
	Wins.Name = "Wins"
	Wins.Value = 0

	local savedLevel = leaderstatsDataStore:GetAsync(Player.UserId.."-Level")

	if savedLevel then
		Level.Value = savedLevel
	end
	
	--
	
	local savedExp = leaderboardDataStore:GetAsync(Player.UserId.."-Exp")

	if savedExp then
		Exp.Value = savedExp
	end
	
	--
	
	local savedRequiredEXP = leaderstatsDataStore:GetAsync(Player.UserId.."-RequiredEXP")

	if savedRequiredEXP then
		RequiredEXP.Value = savedRequiredEXP
	end
	
	--
	
	local savedWins = leaderstatsDataStore:GetAsync(Player.UserId.."-Wins")

	if savedWins then
		Wins.Value = savedWins
	end

end)


game.Players.PlayerRemoving:Connect(function(plr)

	local sucess, err = pcall(function()
		leaderstatsDataStore:SetAsync(plr.UserId.."-Level", plr.leaderstats.Level.Value)
	end)
	
	local sucess, err = pcall(function()
		leaderstatsDataStore:SetAsync(plr.UserId.."-Wins", plr.leaderstats.Wins.Value)
	end)
	
	--
	
	local sucess, err = pcall(function()
		leaderboardDataStore:SetAsync(plr.UserId.."-Exp", plr.leaderboard.Exp.Value)
	end)
	
	--
	
	local sucess, err = pcall(function()
		data:SetAsync(plr.UserId.."-RequiredEXP", plr.RequiredEXP.Value)
         end)
end)
	````
1 Like
#2

You think that’s messy?
It looks fine, but don’t overdo the empty lines. Empty newlines should be used to break up regions of code. Most of this code should be in the same region.
Edit to add: I’m referring to code format only, the other comments are also correct though. You can reduce the number of lines and calls to DataStores.

image
image1621×1011 210 KB 

local dataStoreService = game:GetService("DataStoreService")
local leaderstatsDataStore = dataStoreService:GetGlobalDataStore("leaderstats")
local leaderboardDataStore = dataStoreService:GetGlobalDataStore("leaderboard")
local data = dataStoreService:GetDataStore("Stats")


game.Players.PlayerAdded:Connect(function(Player)
	local leaderstats = Instance.new("Folder", Player)
	leaderstats.Name = "leaderstats"
	local leaderboard = Instance.new("Folder", Player)
	leaderboard.Name = "leaderboard"
	local Level = Instance.new("NumberValue", leaderstats)
	Level.Name = "Level"
	Level.Value = 1
	local Exp = Instance.new("NumberValue", leaderboard)
	Exp.Name = "Exp"
	Exp.Value = 0
	local RequiredEXP = Instance.new("NumberValue", Player)
	RequiredEXP.Name = "RequiredExp"
	RequiredEXP.Value = Level.Value * 100
	local Wins = Instance.new("NumberValue", leaderstats)
	Wins.Name = "Wins"
	Wins.Value = 0

	local savedLevel = leaderstatsDataStore:GetAsync(Player.UserId.."-Level")
	if savedLevel then
		Level.Value = savedLevel
	end
	local savedExp = leaderboardDataStore:GetAsync(Player.UserId.."-Exp")
	if savedExp then
		Exp.Value = savedExp
	end
	local savedRequiredEXP = leaderstatsDataStore:GetAsync(Player.UserId.."-RequiredEXP")
	if savedRequiredEXP then
		RequiredEXP.Value = savedRequiredEXP
	end
	local savedWins = leaderstatsDataStore:GetAsync(Player.UserId.."-Wins")
	if savedWins then
		Wins.Value = savedWins
	end
end)


game.Players.PlayerRemoving:Connect(function(plr)
	local sucess, err = pcall(function()
		leaderstatsDataStore:SetAsync(plr.UserId.."-Level", plr.leaderstats.Level.Value)
	end)
	local sucess, err = pcall(function()
		leaderstatsDataStore:SetAsync(plr.UserId.."-Wins", plr.leaderstats.Wins.Value)
	end)
	local sucess, err = pcall(function()
		leaderboardDataStore:SetAsync(plr.UserId.."-Exp", plr.leaderboard.Exp.Value)
	end)
	local sucess, err = pcall(function()
		data:SetAsync(plr.UserId.."-RequiredEXP", plr.RequiredEXP.Value)
	end)
end)
1 Like
#3

First, don’t use the .Parent argument of Instance.new() as it is slower than setting it through the .Parent property. [Source]

Second, you could get rid of all those if statements in the data loading function by doing

local savedLevel = leaderstatsDataStore:GetAsync(Player.UserId.."-Level") or 1

Level.Value = savedLevel

as if the data was nil it would instead choose 1 as the default level. And you can repeat this for the other stats.

Anyway it would be better to save all the data inside a table instead of having tons of keys for each player and making tons of API requests for each player.

Third, why are you using different pcalls for saving operations? If one is going to fail due to API issues it’s very likely that all the others will as well.

Do this instead:

game.Players.PlayerRemoving:Connect(function(plr)
	local sucess, err = pcall(function()
		leaderstatsDataStore:SetAsync(plr.UserId.."-Level", plr.leaderstats.Level.Value)
        data:SetAsync(plr.UserId.."-RequiredEXP", plr.RequiredEXP.Value)
        leaderboardDataStore:SetAsync(plr.UserId.."-Exp", plr.leaderboard.Exp.Value)
		leaderboardDataStore:SetAsync(plr.UserId.."-Exp", plr.leaderboard.Exp.Value)
	end)
end)

Although you should check if there’s already data saved and if yes then use UpdateAsync() to prevent possible data loss. You were declaring success and result for every pcall but were never using them so it’s useless and can be removed, those variables are not mandatory and you should not just do things by memory.

game.Players.PlayerRemoving:Connect(function(plr)
	pcall(function()
		leaderstatsDataStore:SetAsync(plr.UserId.."-Level", plr.leaderstats.Level.Value)
        data:SetAsync(plr.UserId.."-RequiredEXP", plr.RequiredEXP.Value)
        leaderboardDataStore:SetAsync(plr.UserId.."-Exp", plr.leaderboard.Exp.Value)
		leaderboardDataStore:SetAsync(plr.UserId.."-Exp", plr.leaderboard.Exp.Value)
	end)
end)

Finally this topic should be in #help-and-feedback:code-review

#4

You should be using a single ‘DataStore’ instance for the saving/loading of data.

1 Like