When ever I look at the script it messes with my eyes, like it looks un organize, and is there anything else that I can improve the script or shorten the lines while also keeping it clean.
Old Code
local Trello = require(game.ServerScriptService.ModuleScripts.TrelloAPI)
local httpService = game:GetService("HttpService") -- For decoding and encoding tables
local testCount = game.ServerScriptService:GetAttribute('TestCount')
local dataStoreService = game:GetService('DataStoreService')
local playerData = dataStoreService:GetDataStore('test'..testCount)
local default = {
Armors = "",
Attack = 0,
CurrentArmor = "",
CurrentWeapon = "",
Defense = 0,
Health = 100,
Money = 0,
Rank = "F-1",
SkillAttack = 0,
Speed = 16,
Weapons = ""
}
game.Players.PlayerAdded:Connect(function(player)
local success, data
local attempt = 0
repeat
if attempt == 6 then
Trello:Kick(player, "failed to retrieve data, retrying "..attempt.." Error: "..data, script)
break
elseif attempt > 0 then
warn(player.Name, "failed to retrieve data, retrying", attempt, "\nError:", data)
wait(7)
end
success, data = pcall(function()
return playerData:GetAsync(player.UserId)
end)
attempt += 1
until success
for _, item in pairs(default) do
if data and data[_] then
player:SetAttribute(tostring(_), data[_])
else
player:SetAttribute(tostring(_), item)
end
end
end)
game.Players.PlayerRemoving:Connect(function(player)
local success, response
local hasChange = {}
local attempt = 0
for _, item in pairs(default) do
local attribute = player:GetAttribute(tostring(_))
if attribute ~= item then
hasChange[_] = attribute
end
end
repeat
if attempt == 6 then
Trello:Kick(player, "failed to save data, retrying "..attempt.." Error: "..response, script)
break
elseif attempt > 0 then
warn(player.Name, "failed to save data, retrying", attempt, "\nError:", response)
wait(7)
end
success, response = pcall(function()
return playerData:SetAsync(player.UserId, hasChange)
end)
attempt += 1
until success
end)
Here’s the new code.
New Code
local ProfileService = require(script.ProfileService)
local Trello = require(script.Parent.TrelloAPI)
local count = game.ServerScriptService:GetAttribute("TestCount")
local Profiles = {}
local default = {
Armors = "",
Attack = 0,
Current_Armor = "",
Current_Weapon = "",
Defense = 0,
Health = 100,
Money = 0,
Rank = "F-1",
Skill_Attack = 0,
Speed = 16,
Weapons = ""
}
local ProfileStore = ProfileService.GetProfileStore("Test"..tostring(count), default)
local function PlayerDataLoaded(player)
local profile = Profiles[player]
for name, value in pairs(profile.Data) do
player:SetAttribute(name, value)
end
spawn(function()
while wait(0.1) do
local profile = profile[player]
if profile then
for name, value in pairs(profile.Data) do
local attribute = player:GetAttribute(name)
if attribute ~= value then
player:SetAttribute(name, value)
end
end
end
end
end)
end
local function PlayerAdded(player)
local profile = ProfileStore:LoadProfileAsync("Player_"..player.UserId, "ForceLoad")
if profile then
profile:ListenToRelease(function()
Profiles[player] = nil
Trello:Kick(player, "Your data was loaded remotrly. Please rejoin.", script)
end)
if player:IsDescendantOf(game.Players) then
Profiles[player] = profile
PlayerDataLoaded(player)
else
profile:Release()
end
else
Trello:Kick(player, "Your data did not load.", script)
end
end
local function PlayerRemoving(player)
local profile = Profiles[player]
if profile then
profile:Release()
end
end
game.Players.PlayerAdded:Connect(PlayerAdded)
game.Players.PlayerRemoving:Connect(PlayerRemoving)
for _, player in ipairs(game.Players:GetPlayers()) do
spawn(function()
PlayerAdded(player)
end)
end
function Profiles:GetData(Player : Player)
local profile = Profiles[Player]
print(Player, Profiles)
if profile then
return profile.Data
end
end
return Profiles
Why would I use a module for the data , are you talking about the datastore2? And what does it make unsafe, sorry for having to much question I’m just not really very familiar with datastore.
First I would recommend using a DataStore module like Profile Service or DataStore2. It will relieve you from the hassle of handling saving errors, reconciliation, conflicts, etc. Second, storing your player data at runtime within attributes is not a good idea: not all types of data are supported so it’s limited, and it doesn’t sound like a good practice. Instead, you could use a replication utility like Replica Service.
One more thing: calling a variable _ is a convention that means you’re not using this variable, so if you are using it, do not call it _.
ReplicaService I’m not really familiar with this, is attribute really not a good idea to save and get the data? And for the DataStore, what module should I use Profile Service or DataStore2?
Nvm, I’ll go with Profile Service, because I might use some features that DataStore2 does not have.
When you get the Profile Service Roblox model that Mad Studio provide, there should be inside an example implementation combining Replica Service with Profile Service. It will be very helpful if you don’t understand how to work it out. In my opinion, Replica Service lacks a few more features but nothing that cannot be added on top.
local ProfileService = require(script.ProfileService)
local Trello = require(script.Parent.TrelloAPI)
local count = game.ServerScriptService:GetAttribute("TestCount")
local Profiles = {}
local default = {
Armors = "",
Attack = 0,
Current_Armor = "",
Current_Weapon = "",
Defense = 0,
Health = 100,
Money = 0,
Rank = "F-1",
Skill_Attack = 0,
Speed = 16,
Weapons = ""
}
local ProfileStore = ProfileService.GetProfileStore("Test"..tostring(count), default)
local function PlayerDataLoaded(player)
local profile = Profiles[player]
for name, value in pairs(profile.Data) do
player:SetAttribute(name, value)
end
spawn(function()
while wait(0.1) do
local profile = profile[player]
if profile then
for name, value in pairs(profile.Data) do
local attribute = player:GetAttribute(name)
if attribute ~= value then
player:SetAttribute(name, value)
end
end
end
end
end)
end
local function PlayerAdded(player)
local profile = ProfileStore:LoadProfileAsync("Player_"..player.UserId, "ForceLoad")
if profile then
profile:ListenToRelease(function()
Profiles[player] = nil
Trello:Kick(player, "Your data was loaded remotrly. Please rejoin.", script)
end)
if player:IsDescendantOf(game.Players) then
Profiles[player] = profile
PlayerDataLoaded(player)
else
profile:Release()
end
else
Trello:Kick(player, "Your data did not load.", script)
end
end
local function PlayerRemoving(player)
local profile = Profiles[player]
if profile then
profile:Release()
end
end
game.Players.PlayerAdded:Connect(PlayerAdded)
game.Players.PlayerRemoving:Connect(PlayerRemoving)
for _, player in ipairs(game.Players:GetPlayers()) do
spawn(function()
PlayerAdded(player)
end)
end
function Profiles:GetData(Player : Player)
local profile = Profiles[Player]
print(Player, Profiles)
if profile then
return profile.Data
end
end
return Profiles
Is there anything else that can improved on with? I’m still not sure if I should use attribute to store the data, like what you said that it is not good practice.
And should I use local script on this line, and move the location to replicated storage?
spawn(function()
while wait(0.1) do
local profile = profile[player]
if profile then
for name, value in pairs(profile.Data) do
local attribute = player:GetAttribute(name)
if attribute ~= value then
player:SetAttribute(name, value)
end
end
end
end
end)
Try spacing out some of the functions. I’ll use your game.Players.PlayerAdded as an example:
game.Players.PlayerAdded:Connect(function(player)
local success, data
local attempt = 0
repeat
if attempt == 6 then
Trello:Kick(player, "failed to retrieve data, retrying "..attempt.." Error: "..data, script)
break
elseif attempt > 0 then
warn(player.Name, "failed to retrieve data, retrying", attempt, "\nError:", data)
wait(7)
end
success, data = pcall(function()
return playerData:GetAsync(player.UserId)
end)
attempt += 1
until success
for _, item in pairs(default) do
if data and data[_] then
player:SetAttribute(tostring(_), data[_])
else
player:SetAttribute(tostring(_), item)
end
end
end)
You can instead do this:
game.Players.PlayerAdded:Connect(function(player)
local success, data
local attempt = 0
repeat
if attempt == 6 then
Trello:Kick(player, "failed to retrieve data, retrying "..attempt.." Error: "..data, script)
break
elseif attempt > 0 then
warn(player.Name, "failed to retrieve data, retrying", attempt, "\nError:", data)
wait(7)
end
success, data = pcall(function()
return playerData:GetAsync(player.UserId)
end)
attempt += 1
until success
for _, item in pairs(default) do
if data and data[_] then
player:SetAttribute(tostring(_), data[_])
else
player:SetAttribute(tostring(_), item)
end
end
end)
A simple thing to do is to just add an extra line of whitespace, that can help organize the scripts in some cases.
Honestly there’s no reason to add so many extra spaces. A good practice to keep your code readable is to divide intermediate logical units into functions and put spaces between sub logical units.
Really the only thing that I feel is making it less readable is the inline function definitions, after spawn(). I would suggest making them into a separate function, and using task.spawn() instead. I believe that spawn() runs a wait() before it is executed.
Example:
spawn(function()
PlayerAdded(player)
end)
vs
task.spawn(PlayerAdded, player)
function setAttributes(player)
while wait(0.1) do
local profile = profile[player]
if profile then
for name, value in pairs(profile.Data) do
local attribute = player:GetAttribute(name)
if attribute ~= value then
player:SetAttribute(name, value)
end
end
end
end
end
&
task.spawn(setAttributes, player)
As a side note, I would recommend having a termination condition in the while wait() function like (while profile[player] do) instead of a wait(0.1), since the former would never stop executing, even after the player’s profile has been removed. Just a small little memory leak but better practice
What you are currently doing (although I don’t recommend it) is that you:
Retrieve the player data from datastores
Put that data into player attributes
Upon leaving, you grab the player data from the attributes and save it to datastores
Following this pattern, the only “source of truth” for your player data is the attributes. Therefore, I don’t see where metatables come in as you’re not using actual tables, but attributes.
Hmm, maybe you have not understand this correctly I’m not using attributes to get the data in order to save the player’s data, I’m using the metatable to update the attributes so the player can see the new updated data.
You have said this yourself to use module to save the data, and I’m using ProfileService to save the player’s data. On the ProfileService the way it saves the data is through a table, in other words it does not get the attributes that I have set, I can only get the table and update the attributes with a while loop, or a metatable; the problem with metatable is that from the instruction that I have read on this post, I have to set the player’s data as empty, and yeah it works, but once the player left and rejoin’s, now it’s data is permanently empty.
I think I get what you mean. You want to make changes to the profile.Data table and sync the attributes with the data table, so attributes allow you to easily replicate the data to the client, correct?
Nvm I found a post of how I would be able to present the data to the player without using while loops, and the problem with metatable is that I need to set the data as empty, in order to check if the data has changed.
So on this post I would just need to create a function on the module whenever I want to set a certain data, and on that function also has a set attribute, I dint even though about this.
