1 - It is redundant to have separation comments, because it might be a sign of an organization problem in code. So to start off, you should remove them from your code.
- ------------------------------------------------------------------------------------------------------------------
2 - Fancy comments clutter real information and create noise, you should remove them.
- --//Services//--
+ -- Services
3 - A lot of your code seems to be clustered into deeply nested block. You should split things into functions to make the code easier to digest for the reader.
Code
-- Services
local Players = game:GetService("Players")
local RS = game:GetService("ReplicatedStorage")
-- Tables
local bleedingActive = {}
-- Settings
local StatusEffectsList = {
["Bleeding"] = {
["TickTime"] = 1,
["Damage"] = 1,
},
}
local function bleedingVFX(char, bleedingTime)
bleedingActive[char] = true
local StatusEffectsUi = script.StatusEffectsUi:Clone()
StatusEffectsUi.Parent = char.Head
local BleedingUi = script.Bleeding:Clone()
BleedingUi.Parent = StatusEffectsUi
while bleedingTime > 0 do
bleedingTime = char:GetAttribute("Bleeding")
bleedingTime = bleedingTime - 1
char:SetAttribute("Bleeding", bleedingTime)
BleedingUi.TextLabel.Text = bleedingTime
char.Humanoid:TakeDamage(StatusEffectsList["Bleeding"]["Damage"]) --damaging the character
RS.Events.VFX:FireAllClients("Bleeding", char.HumanoidRootPart) --Emitting particles on all clients
task.wait(StatusEffectsList["Bleeding"]["TickTime"])
end
bleedingActive[char] = nil
StatusEffectsUi:Destroy()
end
local function updateBleeding(char, bleedingTime)
if not bleedingActive[char] then
task.spawn(bleedingVFX)
else
-- Update existing bleeding countdown
local StatusEffectsUi = char.Head:FindFirstChild("StatusEffectsUi")
local BleedingUi = StatusEffectsUi:FindFirstChild("Bleeding")
BleedingUi.TextLabel.Text = bleedingTime
end
end
local function characterAdded(char)
char:GetAttributeChangedSignal("Bleeding"):Connect(function() --when the Bleeding attribute changes
local bleedingTime = char:GetAttribute("Bleeding")
if bleedingTime > 0 then
updateBleeding(char, bleedingTime)
end
end)
end
local function playerAdded(plr)
if plr.Character then
characterAdded(plr.Character)
end
plr.CharacterAdded:Connect(characterAdded)
end
for _, plr in Players:GetPlayers() do
playerAdded(plr)
end
Players.PlayerAdded:Connect(playerAdded)
4 - Unnecessary comments that just repeat what the code is clearly doing.
- char:GetAttributeChangedSignal("Bleeding"):Connect(function() --when the Bleeding attribute changes
+ char:GetAttributeChangedSignal("Bleeding"):Connect(function()
- RS.Events.VFX:FireAllClients("Bleeding", char.HumanoidRootPart) --Emitting particles on all clients
+ RS.Events.VFX:FireAllClients("Bleeding", char.HumanoidRootPart)
- char.Humanoid:TakeDamage(StatusEffectsList["Bleeding"]["Damage"]) --damaging the character
+ char.Humanoid:TakeDamage(StatusEffectsList["Bleeding"]["Damage"])
5 - Shortened variable names, especially those under 3 characters are not understandable to a new reader, or yourself in the future. It forces the reader to find the initialization of the variable to understand its contents.
- local RS = game:GetService("ReplicatedStorage")
- RS.Events.VFX:FireAllClients("Bleeding", char.HumanoidRootPart)
+ local ReplicatedStorage = game:GetService("ReplicatedStorage")
+ ReplicatedStorage.Events.VFX:FireAllClients("Bleeding", char.HumanoidRootPart)
6 - Inconsistent casing in your variable names. You should pick one style, and stick to it.
-- Tables
local bleedingActive = {}
-- Settings
local StatusEffectsList = {
["Bleeding"] = {
["TickTime"] = 1,
["Damage"] = 1,
},
}
- local StatusEffectsList = {
+ local statusEffectsList = {
7 - Some instructions can be reduced.
- bleedingTime = char:GetAttribute("Bleeding")
- bleedingTime = bleedingTime - 1
+ bleedingTime = char:GetAttribute("Bleeding") - 1
8 - Effects don’t need to be scheduled to run immediately. You should consider replacing task.spawn with task.defer.
- task.spawn(bleedingVFX)
+ task.defer(bleedingVFX)
9 - There is possible memory leak if player leaves in the middle of bleeding. You should cleanup bleedingActive entry for the player that is leaving and when their character is being removed.
local function characterRemoving(char)
bleedingActive[char] = nil
end
local function playerAdded(plr)
if plr.Character then
characterAdded(plr.Character)
end
plr.CharacterAdded:Connect(characterAdded)
plr.CharacterRemoving:Connect(characterRemoving)
end
local function playerRemoving(plr)
if plr.Character then
bleedingActive[plr.Character] = nil
end
end
for _, plr in Players:GetPlayers() do
playerAdded(plr)
end
Players.PlayerAdded:Connect(playerAdded)
Players.PlayerRemoving:Connect(playerRemoving)
10 - Lack of types can make code harder to understand because you might not understand what exactly the variable is storing without looking it up in the codebase, and lack of autocomplete can slow you down. Adding even a bit of types, can help with readability, maintenance, and getting mistakes fixed before play testing.
A real world example here is that I forgot that I need to pass arguments to the bleedingVFX function, and if I was to run the game, I would run into an error, but thanks to the types that I added right now and the type checker, I spotted the mistake.
- task.defer(bleedingVFX)
+ task.defer(bleedingVFX, char, bleedingTime)
The final code should look something like this:
-- Services
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
-- Tables
local bleedingActive: { [Model]: boolean } = {}
-- Settings
local statusEffectsList = {
["Bleeding"] = {
["TickTime"] = 1,
["Damage"] = 1,
},
}
local function bleedingVFX(char: Model, bleedingTime: number)
bleedingActive[char] = true
local head = char:FindFirstChild("Head") :: BasePart
local root = char:FindFirstChild("HumanoidRootPart") :: BasePart
local humanoid = char:FindFirstChild("Humanoid") :: Humanoid
local statusEffectsUI = script.StatusEffectsUi:Clone()
statusEffectsUI.Parent = head
local bleedingUI = script.Bleeding:Clone()
bleedingUI.Parent = statusEffectsUI
while bleedingTime > 0 do
bleedingTime = (char:GetAttribute("Bleeding") :: number) - 1
char:SetAttribute("Bleeding", bleedingTime)
bleedingUI.TextLabel.Text = bleedingTime
humanoid:TakeDamage(statusEffectsList["Bleeding"]["Damage"])
ReplicatedStorage.Events.VFX:FireAllClients("Bleeding", root)
task.wait(statusEffectsList["Bleeding"]["TickTime"])
end
bleedingActive[char] = nil
statusEffectsUI:Destroy()
end
local function updateBleeding(char: Model, bleedingTime: number)
if not bleedingActive[char] then
task.defer(bleedingVFX, char, bleedingTime)
else
-- Update existing bleeding countdown
local head = char:FindFirstChild("Head") :: BasePart
local statusEffectsUI = head:FindFirstChild("StatusEffectsUi")
local bleedingUI = statusEffectsUI:FindFirstChild("Bleeding")
bleedingUI.TextLabel.Text = bleedingTime
end
end
local function characterAdded(char: Model)
char:GetAttributeChangedSignal("Bleeding"):Connect(function()
local bleedingTime = char:GetAttribute("Bleeding") :: number
if bleedingTime > 0 then
updateBleeding(char, bleedingTime)
end
end)
end
local function characterRemoving(char: Model)
bleedingActive[char] = nil
end
local function playerAdded(plr: Player)
if plr.Character then
characterAdded(plr.Character)
end
plr.CharacterAdded:Connect(characterAdded)
plr.CharacterRemoving:Connect(characterRemoving)
end
local function playerRemoving(plr: Player)
if plr.Character then
bleedingActive[plr.Character] = nil
end
end
for _, plr in Players:GetPlayers() do
playerAdded(plr)
end
Players.PlayerAdded:Connect(playerAdded)
Players.PlayerRemoving:Connect(playerRemoving)
There might be still bugs in the code, but that would require me to test the script, which I can’t, but hopefully I helped a bit. As for performance, you should not spawn while loops for each character like @AskWisp mentioned, and manipulating UI should be done on client.
