How can I improve my Timer GUI code?

Help and Feedback Code Review
#1

The current code for this is a bit messy, and not very accurate, as it uses a lot of the wait() functions.

How could I improve it?

Feedback will also be much appreciated!

TimeText = script.Parent:WaitForChild("Text")
TimeTitle = script.Parent:WaitForChild("Title")
TimeText.Visible = true
GameManager = game.Workspace:WaitForChild("GameManager")
Timer = GameManager:WaitForChild("GameTime")
Minutes = Timer:WaitForChild("Minutes")
Seconds = Timer:WaitForChild("Seconds")
GameStatus = GameManager:WaitForChild("GameStatus")
RS = game:GetService("RunService")

--Animator = require(script.Parent.Animator)

Minutes.Changed:Connect(function()
	wait(0.5)
	TimeText.TextSize = 40
	wait(0.1)
	TimeText.TextSize = 30
	--Animator.Flash:Play()
end)

Seconds.Changed:Connect(function()
	wait(0.5)
	if Minutes.Value ~= -1 then
		if Seconds.Value < 10 then
			TimeText.Text = Minutes.Value .. ":" .. "0" .. Seconds.Value
		else
			TimeText.Text = Minutes.Value .. ":" .. Seconds.Value
		end
		if Minutes.Value == 0 and Seconds.Value < 30 then
			TimeText.TextSize = 40
			wait(0.1)
			TimeText.TextSize = 30
		end
	else
		TimeText.Text = "0" .. ":" .. "00"
	end
end)

RS.RenderStepped:Connect(function()
	TimeTitle.Text = GameStatus.Value
end)
3 Likes
#2

Make these variables local. Use of global variables is bad practice; they make your code messy.

local TimeText = script.Parent:WaitForChild("Text")
local TimeTitle = script.Parent:WaitForChild("Title")
TimeText.Visible = true

local GameManager = game.Workspace:WaitForChild("GameManager")
local Timer = GameManager:WaitForChild("GameTime")
local Minutes = Timer:WaitForChild("Minutes")
local Seconds = Timer:WaitForChild("Seconds")
local GameStatus = GameManager:WaitForChild("GameStatus")

If you are looking for a timer format, use the string.format specifier "%d:%02d"

The %d means digit, and I can’t explain %02d very well but basically, if the number is only 1 digit long then a 0 will be added at the beginning of it. So your timer would look like 2:05 for example and not 2:5.

TimeText.Text = string.format("%d:%02d", Minutes.Value, Seconds.Value)

No need to run this every render frame; only when the game status changes. No need to reinvent the wheel.

GameStatus.Changed:Connect(function(NewStatus)
    TimeTitle.Text = NewStatus
end)
4 Likes
#3

Also, to simplify your timer itself, you can run it only in seconds. This means you don’t need to deal with setting both Minutes and Seconds, you can just set Seconds to any number without needing to worry about minutes as it will be handled by the following code.

In your TextLabel code, you can calculate the minutes and seconds easily by doing this:

local minutes = math.floor(Seconds.Value / 60)
local seconds = Seconds.Value % 60
TimeText.Text = string.format("%02d:%02d", minutes, seconds)

To calculate minutes: Seconds.Value / 60 finds the number of minutes because in every 60 seconds there is a minute. So if you divide 65 seconds by 60, you get 1.083 (which represents minutes). math.floor is then used to take that number down just 1, as we are showing seconds separately.

To calculate seconds: Seconds.Value % 60 is used , and the % operator means to find the remainder. So what we are doing is trying to find the remander when Seconds.Value is divided by 60. For example, if you try to find the result of 65 % 60, you get 5.

We can now combine the calculate seconds perfectly with the minutes we calculated above, so using the examples, it gives an end result of 1 minute and 5 seconds, which is indeed what 65 seconds represents.

2 Likes
#4

Yeah. I was going to say that as well because that is what I do as well. But I didn’t bother. This is much better though

3 Likes