Is this a good timer that I made?

[louie999]

So I tried to make a simple timer function, so here's what I made:

Code: Select all

activeTimers = {}

function checkNumber(n)
	if type(n) ~= "number" or n < 0 then
		error("[Timer]The delay argument must have a positive number.")
	end
end

function timer(delay, func, ...)
	checkNumber(delay)
	table.insert(activeTimers,{delay = delay, start = 0, func = func, arg = {...}})
end

function deleteTimer(func)
	if func ~= nil then
		for name, times in pairs(activeTimers) do
			if times.func == func then
				table.remove(activeTimers, name)
			end
		end
	else
		for name, a in pairs(activeTimers) do
			table.remove(activeTimers, name)
		end
	end
end

function updateTimers(dt)
	for name, times in pairs(activeTimers) do
		if times.start < times.delay then
			times.start = times.start + dt
		elseif times.start >= times.delay then
			times.func(unpack(times.arg))
			table.remove(activeTimers, name)
		end
	end
end

It works, but I wanna get opinions on this, is it good? does it need to be improved?

[airstruck]

It's a good start. I think there's some room for improvement.

Avoid global variables

Use the local keyword to create local variables and functions, and return a table with the things you want to export. This way everything stays encapsulated; your timer module doesn't interfere with other code and other code doesn't interfere with it.

Use meaningful identifiers

For the most part it's fine, but some identifiers don't convey their purpose well. For example, consider renaming start to elapsed and times to timer.

Use care when modifying arrays during iteration

Code: Select all

local x = { 1, 2, 3, 4, 5 }
for k, v in pairs(x) do
    if v == 3 then 
        table.remove(x, k) 
    end 
    print(v)
end
-- prints 1, 2, 3, 5 ... where'd 4 go?

table.remove moves things around in the array. When you do this while iterating, you may end up skipping some items. Reverse iterate if you want to use table.remove, or ditch the numeric indices and just set things to nil (since you're using pairs anyway).

Be efficient

Code: Select all

for name, a in pairs(activeTimers) do
    table.remove(activeTimers, name)
end

The code above is very inefficient; for every item in the table, the item is removed and every other item is repositioned, until there are no items left. Just write activeTimers = {} instead.

Give me a handle to my timer

Suppose I created two different timers that share the same function (func) but execute at different times, and with different arguments (arg). Now suppose I want to "delete" one timer but not the other. I can't do it, because deleteTimer wants me to pass in func, and both timers share func. Consider having timer return the table that was inserted into activeTimers, and have deleteTimer accept that table (the actual timer) rather than the callback function that was attached to it.

[louie999]

I tried reworking on it, maybe this is better?

Code: Select all

local timers = {}
local counter = 0

local function checkNumber(n)
	if type(n) ~= "number" or n < 0 then
		return false
	end
	return true
end

function freeTimer(idd)
	for _, timer in pairs(timers) do
		if idd == timer.id then
			timers[timer.id] = nil
		end
	end
end

function addTimer(delay, func, count, ...)
	if not checkNumber(delay) then error("[Timer]>delay< must be a positive number") end
	counter = counter + 1
	timers[counter] = {
	id = counter,
	delay = delay, 
	elapsed = 0, 
	func = func, 
	count = count,
	arg = {...}
	}
end

function updateTimers(dt)
	for handle, timer in pairs(timers) do
		if timer.count > 0 then
			if timer.elapsed < timer.delay then
				timer.elapsed = timer.elapsed + dt
			elseif timer.elapsed >= timer.delay then
				timer.count = timer.count - 1
				timer.elapsed = 0
				timer.func(unpack(timer.arg))
				counter = counter - 1
			end
		elseif timer.count == 0 then
			timers[timer.id] = nil
		elseif timer.count <= -1 then
			if timer.elapsed < timer.delay then
				timer.elapsed = timer.elapsed + dt
			elseif timer.elapsed >= timer.delay then
				timer.elapsed = 0
				timer.func(unpack(timer.arg))
				timers[timer.id] = nil
				counter = counter - 1
			end
		end
	end
end

I tested it and it works, though I'm still not 100% sure if it's not gonna have any bugs.

[airstruck]

It's looking better. You're still leaving a lot of stuff in the global scope, though.

Try something like this:

Code: Select all

-- timer.lua

local Timer = {}

function Timer.free (id)
    -- ...
end

function Timer.add (delay, func, count, ...)
    -- ...
end

function Timer.update (dt)
    -- ...
end

return Timer

And when you use your timer:

Code: Select all

-- main.lua

local Timer = require 'timer'

In other words, instead of leaving a bunch of functions in the global scope, put them in a table and then return that table from your timer module. The goal should be to have the module completely self-contained (no global variables at all).

[louie999]

Ok I have put all the necessary functions in a table:

Code: Select all

local timer = {}
local timers = {}
local counter = 0

local function checkNumber(n)
	if type(n) ~= "number" or n < 0 then
		return false
	end
	return true
end

function timer.Free(idd)
	for _, t in pairs(timers) do
		if idd == timer.id then
			timers[timer.id] = nil
		end
	end
end

function timer.Add(delay, func, count, ...)
	if not checkNumber(delay) then error("[Timer]>delay< must be a positive number") end
	counter = counter + 1
	timers[counter] = {
	id = counter,
	delay = delay, 
	elapsed = 0, 
	func = func, 
	count = count,
	arg = {...}
	}
end

function timer.updateAll(dt)
	for handle, timer in pairs(timers) do
		if timer.count > 0 then
			if timer.elapsed < timer.delay then
				timer.elapsed = timer.elapsed + dt
			elseif timer.elapsed >= timer.delay then
				timer.count = timer.count - 1
				timer.elapsed = 0
				timer.func(unpack(timer.arg))
				counter = counter - 1
			end
		elseif timer.count == 0 then
			timers[timer.id] = nil
		elseif timer.count <= -1 then
			if timer.elapsed < timer.delay then
				timer.elapsed = timer.elapsed + dt
			elseif timer.elapsed >= timer.delay then
				timer.elapsed = 0
				timer.func(unpack(timer.arg))
				timers[timer.id] = nil
				counter = counter - 1
			end
		end
	end
end

return timer