Hello developers!
I’m working on an honor system for my game, and I was wondering if I could use some help
The code already works, that’s why its here (duh) but it seems hard to read and to work with. How can I make this more organized? if so, what should I do?
Thanks!
local DSS = game:GetService("DataStoreService")
local honorData = DSS:GetDataStore("honorStore")
local function honorAdd(plr, honor)
plr.leaderstats.Honor.Value += 1
end
game.Players.PlayerAdded:Connect(function(plr)
local ls = Instance.new("Folder", plr)
ls.Name = "leaderstats"
local honor = Instance.new("IntValue", ls)
honor.Name = "Honor"
honor.Value = 0
local data
local suc, rep = pcall(function()
data = honorData:GetAsync(plr.UserId.."-honor")
end)
if suc then
honor.Value = data
end
while wait(600) do
if plr.Team == game.Teams.Army then
honorAdd(plr, honor)
end
end
end)
game.Players.PlayerRemoving:Connect(function(plr)
pcall(function()
honorData:SetAsync(plr.UserId.."-honor",plr.leaderstats.Honor.Value)
end)
end)
They aren’t wrong. You should NEVER use while wait() do
Worth watching this video as to why. Video talks about exactly what @PoIemicist has mentioned, but it also states why you should not do while wait() do
Please read my posts properly before making a point that I’ve answered already in my previous post.
You should NEVER use while wait() do
You should only replace wait with a better function in certain cirumstances (see below) or when you want a higher and reliable accuracy which isn’t always needed. I can see that you haven’t even seen the video fully. If you did, you would have never made this reply.
Even if you care for accuracies, wait will be accurate for the most time. Cases where frame rates are dropping or there are too many threads to be resumed in the queue, will cause wait to throttle, and so will HeartbeatWait, except that it won’t throttle as bad as wait since its 60hz (even more on client if on fps unlocker). Hence why its superior than wait, but not necessarily always.
The video explains why you should never use wait in certain circumstances. See this:
You can see that that the scheduler resumes any yielded threads by wait right AFTER replication recieve jobs and screen drawing is done before that. So using wait to yield and then do any stuff related to the camera will not be smooth and will cause stutters because of the priority.
Hence why you shouldn’t use wait in this case but rather RenderStepped:Wait or RenderStepped:Connect to perform work related to the camera.
Your code is ok, but you should remember to keep your variables consistent. Shortening some variables like ls and suc isn’t a great practice. (at least not in my opinion)
Your way of pcall’ing is the way that I see a lot of people do it, and it does work, but I always recommend returning the data, and using the result. A common misconception I see, is that the second variable returned is the error, but it’ll only be so if the pcall catches an error. What do I mean by this?
local success, data = pcall(function()
return DataStore:GetAsync(key)
end)
The data will be the data you need, as long as it’s returned (which it only won’t if an error is experienced)! No need to do those unnecessary data variables.
The while wait() do is already being discussed in this thread, so I wont bring that up
honorAdd is quite an unnecessary function, since it doesn’t reduce the amount of code needed, and it’s rather easy to understand. (but I appreciate the camelCase)
Your whitespace is very consistent - 'cause there isn’t any. I’m not gonna be the one to judge use of whitespace since I sometimes look at the code I wrote an hour ago and think: “why didn’t I press Enter here?”
Sorry if this reply was scuffed, DevForum on phone is pretty confusing
If the call to load your data fails, players’ data can be wiped because you save when they leave the game without checking whether or not the load was successful.
You have a memory leak with your loop, because it will continue running even after the player leaves the game. You can check whether or not the player is still in the game and break out of the loop if they have left.
You should not be running that while loop inside the PlayerAdded event handler. It will start a loop for every single player and can memory leak by a reference to the player.
You should have the loop to increment honour outside of any event handler and it should loop over every player. You can either run in in a coroutine or bind it to a lifecycle event like others have mentioned above.
coroutine.resume(coroutine.create(function()
while wait(600) do
for _, player in pairs(game.Players:GetPlayers()) do
if player.Team == game.Teams.Army then
player.leaderstats.Honor.Value += 1
end
end
end
end)
You also may want to check if player.leaderstats.Honor exists before incrementing the value.
