So I recently made a simple Timer class for my game (this timer counts down not up btw) which uses the Trove and Signal classes as well as coroutines.
I was wondering how I could improve it because I feel like there are probably more efficient ways of doing this. I’m just worried about the performance and safety of this because I’m still learning how to properly use coroutines as well as Signals and the Trove class.
If anyone has any suggestions on how to more efficiently use coroutines in this case (if I even needed them in the first place) and if there are better ways I can use Trove and Signals. Or any other improvements in general
Of course, as always, I appreciate any suggestions/feedback!
--//References
local modules = script.Parent.Parent
local utilMODS = modules.Utility
--//Modules
local Trove = require(utilMODS.Trove)
local Signal = require(utilMODS.Signal)
--//Class
local Timer = {}
Timer.__index = Timer
function Timer.new(duration)
local self = setmetatable({}, Timer)
self._trove = Trove.new()
self._originalDuration = duration
self.Paused = true
self.Duration = duration
self.Completed = self._trove:Add(Signal.new())
self.Updated = self._trove:Add(Signal.new())
self._trove:Connect(self.Completed, function()
print("Timer has completed!")
end)
self._trove:Connect(self.Updated, function() -- so far this is only used for testing purposes but might have actual use later on
print(self.Duration)
end)
return self
end
function Timer:Start()
if not self.Paused then
warn("Timer is already running")
return
end
self.Paused = false
self.Thread = coroutine.create(function()
repeat
if self.Paused then
coroutine.yield()
end
self.Duration -= 1
self.Updated:Fire()
task.wait(1)
until self.Duration <= 0
self.Completed:Fire()
self:Destroy()
end)
coroutine.resume(self.Thread)
end
function Timer:Pause()
self.Paused = true
end
function Timer:Unpause()
self.Paused = false
coroutine.resume(self.Thread)
end
function Timer:Reset()
self.Duration = self._originalDuration
end
function Timer:Destroy()
coroutine.yield(self.Thread)
coroutine.close(self.Thread)
self._trove:Clean()
end
return Timer
You can always use guard clauses to check if the timer is already running, i.e Timer:Start(), add some error handling (asset, warn, error etc.), use consistent naming conventions; you use both pascalCase and CamelCase, it’s best to just stick to one.
First of all, you should know that coroutine.yield() does not take a thread argument. It yields the thread it was called in, not the thread you gave to it or anything. So make sure to fix that in Timer:Destroy(), you can just get rid of the coroutine.yield(self.Thread) line and it’ll still work since coroutine.close can also close running coroutines.
Normally you’d want to make a single thread that runs while a Timer has started to handle all the Timers at the same time. This would improve performance since you wouldn’t be making multiple threads for it. For self.Paused you’d want to remove the Timer from the pool that is looped through by the thread that handles all the Timers.
You should also just use task.wait() instead of task.wait(1) unless you don’t think anyone will ever care about accuracy. You’d want to change it to self.Duration -= task.wait() since task.wait() returns the time passed.
Thanks for the suggestions but I am a little confused about how to make it a single thread that handles all timers instead of multiple threads. are u saying I should completely get rid of the coroutines or am I misunderstanding?
Also P.S, your right about the coroutine.yield() not needing an argument but I can’t get rid of that line because coroutine.close() cannot close a running coroutine, it will throw an error and the thread won’t close
The task.wait() suggestion worked great though so thanks for that
Edited your script to add the second thing I mentioned. The point is that one thread handles all the running timers at the same time. Can’t test it but I hope you get the idea.
--//References
local modules = script.Parent.Parent
local utilMODS = modules.Utility
local RunningTimers = {}
--//Modules
local Trove = require(utilMODS.Trove)
local Signal = require(utilMODS.Signal)
--//Timer Handler
local TimerHandler = task.spawn(function()
while true do
local TimePassed = task.wait()
for Index, Timer in RunningTimers do
if Timer.Paused then
RunningTimers[Index] = nil
return
end
Timer.Duration -= TimePassed
Timer.Updated:Fire()
end
end
end)
--//Class
local Timer = {}
Timer.__index = Timer
function Timer.new(duration)
local self = setmetatable({}, Timer)
self._trove = Trove.new()
self._originalDuration = duration
self.Paused = true
self.Duration = duration
self.Completed = self._trove:Add(Signal.new())
self.Updated = self._trove:Add(Signal.new())
self._trove:Connect(self.Completed, function()
print("Timer has completed!")
end)
self._trove:Connect(self.Updated, function() -- so far this is only used for testing purposes but might have actual use later on
print(self.Duration)
end)
return self
end
function Timer:Start()
self.Paused = false
if table.find(RunningTimers, self) then
warn("Timer is already running")
return
end
table.insert(RunningTimers, self)
end
function Timer:Pause()
self.Paused = true
end
function Timer:Unpause()
self.Paused = false
coroutine.resume(self.Thread)
end
function Timer:Reset()
self.Duration = self._originalDuration
end
function Timer:Destroy()
self.Paused = true
self._trove:Clean()
end
return Timer