Feedback on health bar GUI

Help and Feedback Code Review
#1

I have a health bar GUI but I’m not sure how good it is. Should I even be using a loop? Or could I use a Changed event to update the GUI? What else could be improved though?

local player = game.Players.LocalPlayer
local bar = script.Parent.Foreground.FrontBar
local text = script.Parent.Foreground.FrontBar.HealthText

player.CharacterAdded:Connect(function(character)
    repeat wait() until character.Humanoid
    bar.Size = UDim2.new(0, 206, 0, 16)
    text.Text = "100%"
    bar.BackgroundTransparency = 0
    
    while character:FindFirstChild("Humanoid").Health > 0 do
        local XOffset = math.floor((character.Humanoid.Health / character.Humanoid.MaxHealth) * 206)
        local percentage = math.floor((character.Humanoid.Health / character.Humanoid.MaxHealth) * 100)
        bar.Size = UDim2.new(0, XOffset, 0, 16)
        text.Text = percentage.."%"
        
        if percentage > 75 then
            bar.BackgroundColor3 = Color3.new(0 / 255, 255 / 255, 0 / 255)
            bar.BorderColor3 = Color3.new(0 / 255, 255 / 255, 0 / 255)
            
        elseif percentage > 50 and percentage <= 75 then
            
            --bar.BackgroundColor3 = Color3.new(223 / 255, 160 / 255, 13 / 255)
            --bar.BorderColor3 = Color3.new(223 / 255, 160 / 255, 13 / 255)
            bar.BackgroundColor3 = Color3.new(255 / 255, 255 / 255, 0 / 255)
            bar.BorderColor3 = Color3.new(255 / 255, 255 / 255, 0 / 255)
            
        elseif percentage > 25 and percentage <= 50 then
            
            --bar.BackgroundColor3 = Color3.new(140 / 255, 98 / 255, 7 / 255)
            --bar.BorderColor3 = Color3.new(140 / 255, 98 / 255, 7 / 255)
            bar.BackgroundColor3 = Color3.new(190 / 255, 80 / 255, 0 / 255)
            bar.BorderColor3 = Color3.new(190 / 255, 80 / 255, 0 / 255)
            
        elseif (percentage > 0 or percentage < 0 or percentage == 0) and percentage <= 25 then
            
            bar.BackgroundColor3 = Color3.new(255 / 255, 0 / 255, 0 / 255)
            bar.BorderColor3 = Color3.new(255 / 255, 0 / 255, 0 / 255)
        end
        
        wait()
    end
    
    bar.Size = UDim2.new(0, 0, 0, 16)
    text.Text = "0%"
    bar.BackgroundTransparency = 1
end)

What about the if elseif statements? Do I actually need to have the upper limit there?

9 Likes
#2

You should never be using infinate loops when there are events available for the task. You should first remove these from you code for a start. Then focus on removing the dupliacate code.

6 Likes
#3

Try for character:FindFirstChild("Humanoid"):GetPropertyChangedSignal("Health") event instead of while character:FindFirstChild("Humanoid").Health > 0 do. Also include if statements if the health runs below 0.

The paragraph above also requires the block of code below here from the script above to be moved into the end of the if statements.

else
    bar.Size = UDim2.new(0, 0, 0, 16)
    text.Text = "0%"
    bar.BackgroundTransparency = 1
end

Also repeat wait() until character.Humanoid to character:WaitForChild("Humanoid"). Index it with a variable such as humanoid.

6 Likes
#4 
if percentage > 75 then
    --Stuff.
    
elseif percentage > 50 and percentage <= 75 then
    
    --Stuff.
end

Do I actually need to specify the upper limit “and percentage <= 75” for the elseif statements? And not just for this context but in general like when using a random selector with four equally probable outcomes.

1 Like
#5

That’ll be easy to test.

Just by reading it, however, it looks like that’s redundant because the elseif statement can only be reached if then first statement wasn’t reached. Therefore, if that statement is run, we know that it must be less than or equal to 75.

2 Likes
#6

Oh no. Now that I’ve changed it to a GetPropertyChangedSignal event to update instead of a loop, it seems that only health updates on the client would fire the event and not health updates on the server, which is extremely bad now…

1 Like
#7

Humanoid.HealthChanged has the behavior you want

GetPropertyChangedSignal() doesn’t return a signal on the client which fires for some properties of a Humanoid (notably Health, Walkspeed, etc.) when they’re changed on the server - seems like this is probably a bug

5 Likes
#8

Looks like everybody else that replied gave good advice.
However, you can optimize it even more.

Instead of doing:

if percentage > 75 then
    --Stuff.
    
elseif percentage > 50 and percentage <= 75 then
    
    --Stuff.
end

do:

bar.BackgroundColor3 = ((percentage > 75 and Color3.fromRGB(0,255,0)) or ((percentage > 50 and percentage <= 75) and Color3.fromRGB(255,255,0)) or ((percentage > 25 and percentage <= 50) and Color3.fromRGB(190,80,0)) or Color3.fromRGB(255,0,0))

Do the same with the border color. This makes it where you don’t need to write the ifs and thens, and it just compresses the same thing you are asking.

Edit: This is untested and may contain a bug or two. Lemme know if so.

1 Like
#9

In general when programming you want to favor readability. This code is not actually that much smaller and you’re making it much more harder to read for no particular reason here. The experienced programmer would always pick the top option with if/then over the bottom option.

4 Likes
#10

Alright, good to know.
I guess ignore what I said XD

1 Like
#11

Here to give my two cents.

You can also order it in increasing order so you don’t need the or statement.

if percentage < 25 then. -- [0,25)
    --stuff
elseif percentage < 50 then   -- [25,50)
    -- something else
elseif percentage < 75 then -- [50,75)
    -- something else
else -- [75,100)
   -- something else
end

Or if you wanted to be fancy (and I would really only do this if you had a load of conditioons) you could format it in a table.

3 Likes
#12

Looks like you have a good system going on!

Just like @Operatik suggested, using while loops to constantly check a player’s health even when it is unchanged is unnecessary and can potentially slow down performance.

Another thing I’d like to add is to use scale when applying sizes and positions to UI objects. Using offset means that the element will say the same size in pixels regardless of the user’s screen size. This means that the UI element may look very large in mobile devices and very small in a larger screen.

Other than that, the overall structure of the script is very nice.

1 Like
#13

Hmm… I read this the other time but now I’m confused how this works. Shouldn’t that return a boolean value?

2 Likes
#14

Nope. In lua, you can do:
local value (bool==true and wantedValue or notWantedValue)
-- If bool is equal to true, it will return wantedValue, other it will return notWantedValue

1 Like