Use table indices as IDs

This significantly optimizes a lot of operations and removes redundancy.

6 files changed, 124 insertions, 130 deletions

diff --git a/api.lua b/api.lua
index 9404241..88c8b1e 100644
--- a/api.lua
+++ b/api.lua
@@ -1,4 +1,3 @@
---TODO Less code duplication
 
 -- Checks if the area is unprotected or owned by you
 function areas:canInteract(pos, name)
@@ -28,9 +27,7 @@ function areas:getNodeOwners(pos)
 		if pos.x >= area.pos1.x and pos.x <= area.pos2.x and
 		   pos.y >= area.pos1.y and pos.y <= area.pos2.y and
 		   pos.z >= area.pos1.z and pos.z <= area.pos2.z then
-			if area.owner ~= nil then
-				table.insert(owners, area.owner)
-			end
+			table.insert(owners, area.owner)
 		end
 	end
 	return owners
diff --git a/chatcommands.lua b/chatcommands.lua
index 58978f1..e951142 100644
--- a/chatcommands.lua
+++ b/chatcommands.lua
@@ -1,3 +1,4 @@
+
 minetest.register_chatcommand("protect", {
 	params = "<AreaName>",
 	description = "Protect your own area",
@@ -7,10 +8,8 @@ minetest.register_chatcommand("protect", {
 			minetest.chat_send_player(name, 'Invalid usage, see /help protect')
 			return
 		end
-		local pos1, pos2 = {}, {}
-		if areas:getPos1(name) and areas:getPos2(name) then
-			pos1 = areas:getPos1(name)
-			pos2 = areas:getPos2(name)
+		local pos1, pos2 = areas:getPos1(name), areas:getPos2(name)
+		if pos1 and pos2 then
 			pos1, pos2 = areas:sortPos(pos1, pos2)
 		else
 			minetest.chat_send_player(name, 'You need to select an area first')
@@ -30,10 +29,10 @@ minetest.register_chatcommand("protect", {
 			return
 		end
 
-		areas:add(name, param, pos1, pos2, nil)
+		local id = areas:add(name, param, pos1, pos2, nil)
 		areas:save()
 
-		minetest.chat_send_player(name, "Area protected")
+		minetest.chat_send_player(name, "Area protected. ID: "..id)
 end})
 
 
@@ -51,10 +50,8 @@ minetest.register_chatcommand("set_owner", {
 			return
 		end
 
-		local pos1, pos2 = {}, {}
-		if areas:getPos1(name) and areas:getPos2(name) then
-			pos1 = areas:getPos1(name)
-			pos2 = areas:getPos2(name)
+		local pos1, pos2 = areas:getPos1(name), areas:getPos2(name)
+		if pos1 and pos2 then
 			pos1, pos2 = areas:sortPos(pos1, pos2)
 		else
 			minetest.chat_send_player(name, "You need to select an area first")
@@ -67,18 +64,18 @@ minetest.register_chatcommand("set_owner", {
 			return
 		end
 
-		minetest.log("action", name.." runs /set_owner. Owner="..ownername..
-				" AreaName="..areaname..
-				" StartPos="..minetest.pos_to_string(pos1)..
-				" EndPos="  ..minetest.pos_to_string(pos2))
+		minetest.log("action", name.." runs /set_owner. Owner = "..ownername..
+				" AreaName = "..areaname..
+				" StartPos = "..minetest.pos_to_string(pos1)..
+				" EndPos = "  ..minetest.pos_to_string(pos2))
 
-		areas:add(ownername, areaname, pos1, pos2, nil)
+		local id = areas:add(ownername, areaname, pos1, pos2, nil)
 		areas:save()
 	
 		minetest.chat_send_player(ownername,
-				"You have been granted control over an area."
-				.." Type /list_areas to show your areas.")
-		minetest.chat_send_player(name, "Area protected")
+				"You have been granted control over area #"..
+				id..". Type /list_areas to show your areas.")
+		minetest.chat_send_player(name, "Area protected. ID: "..id)
 end})
 
 
@@ -97,10 +94,8 @@ minetest.register_chatcommand("add_owner", {
 			return
 		end
 
-		local pos1, pos2 = {}, {}
-		if areas:getPos1(name) and areas:getPos2(name) then
-			pos1 = areas:getPos1(name)
-			pos2 = areas:getPos2(name)
+		local pos1, pos2 = areas:getPos1(name), areas:getPos2(name)
+		if pos1 and pos2 then
 			pos1, pos2 = areas:sortPos(pos1, pos2)
 		else
 			minetest.chat_send_player(name, 'You need to select an area first')
@@ -127,13 +122,13 @@ minetest.register_chatcommand("add_owner", {
 			return
 		end
 
-		areas:add(ownername, areaname, pos1, pos2, pid)
+		local id = areas:add(ownername, areaname, pos1, pos2, pid)
 		areas:save()
 
 		minetest.chat_send_player(ownername,
-				"You have been granted control over an area."
-				.." Type /list_areas to show your areas.")
-		minetest.chat_send_player(name, "Area protected.")
+				"You have been granted control over area #"..
+				id..". Type /list_areas to show your areas.")
+		minetest.chat_send_player(name, "Area protected. ID: "..id)
 end})
 
 
@@ -150,9 +145,7 @@ minetest.register_chatcommand("rename_area", {
 		end
 
 		id = tonumber(id)
-		index = areas:getIndexById(id)
-
-		if not index then
+		if not id then
 			minetest.chat_send_player(name, "That area doesn't exist.")
 			return
 		end
@@ -162,7 +155,7 @@ minetest.register_chatcommand("rename_area", {
 			return
 		end
 
-		areas.areas[index].name = newName
+		areas.areas[id].name = newName
 		areas:save()
 		minetest.chat_send_player(name, "Area renamed.")
 end})
@@ -178,11 +171,12 @@ minetest.register_chatcommand("find_areas", {
 					"A regular expression is required.")
 			return
 		end
+
 		local found = false
-		for _, area in pairs(areas.areas) do
-			if areas:isAreaOwner(area.id, name) and
-			   areas:toString(area):find(param) then
-				minetest.chat_send_player(name, areas:toString(area))
+		for id, area in pairs(areas.areas) do
+			if areas:isAreaOwner(id, name) and
+			   areas:toString(id):find(param) then
+				minetest.chat_send_player(name, areas:toString(id))
 				found = true
 			end
 		end
@@ -205,10 +199,10 @@ minetest.register_chatcommand("list_areas", {
 			minetest.chat_send_player(name,
 					"Showing your areas.")
 		end
-		for _, area in pairs(areas.areas) do
-			if admin or areas:isAreaOwner(area.id, name) then
+		for id, area in pairs(areas.areas) do
+			if admin or areas:isAreaOwner(id, name) then
 				minetest.chat_send_player(name,
-						areas:toString(area))
+						areas:toString(id))
 			end
 		end
 end})
@@ -227,16 +221,15 @@ minetest.register_chatcommand("recursive_remove_areas", {
 			return
 		end
 
-		if areas:isAreaOwner(id, name) then
-			areas:remove(id, true)
-			areas:sort()
-			areas:save()
-		else
+		if not areas:isAreaOwner(id, name) then
 			minetest.chat_send_player(name, "Area "..id
 					.." does not exist or is"
 					.." not owned by you.")
 			return
 		end
+
+		areas:remove(id, true)
+		areas:save()
 		minetest.chat_send_player(name, "Removed area "..id
 				.." and it's sub areas.")
 end})
@@ -254,16 +247,15 @@ minetest.register_chatcommand("remove_area", {
 			return
 		end
 
-		if areas:isAreaOwner(id, name) then
-			areas:remove(id, false)
-			areas:sort()
-			areas:save()
-		else
+		if not areas:isAreaOwner(id, name) then
 			minetest.chat_send_player(name, "Area "..id
 					.." does not exist or"
 					.." is not owned by you")
 			return
 		end
+
+		areas:remove(id)
+		areas:save()
 		minetest.chat_send_player(name, 'Removed area '..id)
 end})
 
@@ -296,8 +288,7 @@ minetest.register_chatcommand("change_owner", {
 					.." or is not owned by you.")
 			return
 		end
-		local index = areas:getIndexById(id)
-		areas.areas[index].owner = new_owner
+		areas.areas[id].owner = new_owner
 		areas:save()
 		minetest.chat_send_player(name, 'Owner changed.')
 		minetest.chat_send_player(new_owner,
diff --git a/init.lua b/init.lua
index d3ffd74..1aca354 100644
--- a/init.lua
+++ b/init.lua
@@ -21,12 +21,17 @@ areas:load()
 minetest.register_privilege("areas", {description = "Can administer areas"})
 
 if not minetest.registered_privileges[areas.self_protection_privilege] then
-	minetest.register_privilege(areas.self_protection_privilege,
-		{description = "Can protect areas"})
+	minetest.register_privilege(areas.self_protection_privilege, {
+		description = "Can protect areas",
+	})
+end
+
+for _, area in pairs(areas.areas) do
+	area.id = nil
 end
 
 if minetest.setting_getbool("log_mod") then
 	local diffTime = os.clock() - areas.startTime
-	print("[areas] loaded in "..diffTime.."s.")
+	minetest.log("action", "areas loaded in "..diffTime.."s.")
 end
 
diff --git a/internal.lua b/internal.lua
index e57cfd8..f52b954 100644
--- a/internal.lua
+++ b/internal.lua
@@ -1,3 +1,4 @@
+
 function areas:player_exists(name)
 	return minetest.auth_table[name] ~= nil
 end
@@ -20,45 +21,59 @@ function areas:load()
 		return err
 	end
 	self.areas = minetest.deserialize(file:read("*a"))
-	if type(self.areas) ~= "table" then self.areas = {} end
+	if type(self.areas) ~= "table" then
+		self.areas = {}
+	end
 	file:close()
 end
 
--- Shorter than the table function
+-- Finds the first usable index in a table
+-- Eg: {[1]=false,[4]=true} -> 2
+local function findFirstUnusedIndex(t)
+	for i = 1, #t + 1 do
+		if t[i] == nil then
+			return i
+		end
+	end
+end
+
+-- Add a area, returning the new area's id.
 function areas:add(owner, name, pos1, pos2, parent)
-	table.insert(areas.areas, {id=table.maxn(self.areas)+1, name=name,
-			pos1=pos1, pos2=pos2, owner=owner, parent=parent})
+	local id = findFirstUnusedIndex(self.areas)
+	self.areas[id] = {name=name, pos1=pos1, pos2=pos2, owner=owner,
+			parent=parent}
+	return id
 end
 
 -- Remove a area, and optionally it's children recursively.
 -- If a area is deleted non-recursively the children will
 -- have the removed area's parent as their new parent.
-function areas:remove(id, recurse)
+function areas:remove(id, recurse, secondrun)
 	if recurse then
 		-- Recursively find child entries and remove them
 		local cids = self:getChildren(id)
 		for _, cid in pairs(cids) do
-			self:remove(cid, true)
+			self:remove(cid, true, true)
 		end
 	else
 		-- Update parents
-		local parent = self:getAreaById(id).parent
+		local parent = self.areas[id].parent
 		local children = self:getChildren(id)
-		for _, child in pairs(children) do
+		for _, cid in pairs(children) do
 			-- The subarea parent will be niled out if the
 			-- removed area does not have a parent
-			areas.areas[self:getIndexById(child)].parent = parent
+			self.areas[cid].parent = parent
 
 		end
 	end
 
 	-- Remove main entry
-	table.remove(self.areas, self:getIndexById(id))
+	self.areas[id] = nil
 end
 
 -- Checks if a area between two points is entirely contained by another area
 function areas:isSubarea(pos1, pos2, id)
-	local area = areas:getAreaById(id)
+	local area = self.areas[id]
 	if not area then
 		return false
 	end
@@ -76,9 +91,9 @@ end
 -- Returns a table (list) of children of an area given it's identifier
 function areas:getChildren(id)
 	local children = {}
-	for _, area in pairs(self.areas) do
+	for cid, area in pairs(self.areas) do
 		if area.parent and area.parent == id then
-			table.insert(children, area.id)
+			table.insert(children, cid)
 		end
 	end
 	return children
@@ -140,57 +155,43 @@ function areas:canPlayerAddArea(pos1, pos2, name)
 	return true, ""
 end
 
--- Given a area returns a string in the format:
+-- Given a id returns a string in the format:
 -- "name [id]: owner (x1, y1, z1) (x2, y2, z2) -> children"
-function areas:toString(area)
-	local message = area.name..
-		" ["..area.id.."]: "..area.owner.." "..
-		minetest.pos_to_string(area.pos1).." "..
-		minetest.pos_to_string(area.pos2)
+function areas:toString(id)
+	local area = self.areas[id]
+	local message = ("%s [%d]: %s %s %s"):format(
+		area.name, id, area.owner,
+		minetest.pos_to_string(area.pos1),
+		minetest.pos_to_string(area.pos2))
 
 	local children = areas:getChildren(id)
 	if #children > 0 then
-		message = message..
-		" -> "..table.concat(children, ", ")
+		message = message.." -> "..table.concat(children, ", ")
 	end
-	return message		
-end
-
--- Returns a area given it's identifier
-function areas:getAreaById(id)
-	if not self.areas[id] then
-		return nil
-	end
-	assert(self.areas[id].id == id)
-	return self.areas[id]
-end
-
--- Returns a table index for an area given it's identifier
-function areas:getIndexById(id)
-	if not self.areas[id] then
-		return nil
-	end
-	assert(self.areas[id].id == id)
-	return id
+	return message
 end
 
 -- Re-order areas in table by their identifiers
 function areas:sort()
+	local sa = {}
 	for k, area in pairs(self.areas) do
-		if area.id ~= k then
+		if not area.parent then
+			table.insert(sa, area)
+			local newid = #sa
 			for _, subarea in pairs(self.areas) do
-				if subarea.parent == area.id then
-					subarea.parent = k
+				if subarea.parent == k then
+					subarea.parent = newid
+					table.insert(sa, subarea)
 				end
 			end
-			area.id = k
 		end
 	end
+	self.areas = sa
 end
 
 -- Checks if a player owns an area or a parent of it
 function areas:isAreaOwner(id, name)
-	local cur = self:getAreaById(id)
+	local cur = self.areas[id]
 	if cur and minetest.check_player_privs(name, {areas=true}) then
 		return true
 	end
@@ -198,7 +199,7 @@ function areas:isAreaOwner(id, name)
 		if cur.owner == name then
 			return true
 		elseif cur.parent then
-			cur = self:getAreaById(cur.parent)
+			cur = self.areas[cur.parent]
 		else
 			return false
 		end
diff --git a/legacy.lua b/legacy.lua
index 82fa4ee..00e6894 100644
--- a/legacy.lua
+++ b/legacy.lua
@@ -22,21 +22,22 @@ minetest.register_chatcommand("legacy_load_areas", {
 		minetest.chat_send_player(name, "Legacy file loaded.")
 
 		for k, area in pairs(areas.areas) do
-			--New position format
-			areas.areas[k].pos1 = {x=area.x1, y=area.y1, z=area.z1}
-			areas.areas[k].pos2 = {x=area.x2, y=area.y2, z=area.z2}
+			-- New position format
+			area.pos1 = {x=area.x1, y=area.y1, z=area.z1}
+			area.pos2 = {x=area.x2, y=area.y2, z=area.z2}
 
-			areas.areas[k].x1, areas.areas[k].y1,
-			areas.areas[k].z1, areas.areas[k].x2,
-			areas.areas[k].y2, areas.areas[k].z2 =
+			area.x1, area.y1, area.z1,
+			area.x2, area.y2, area.z2 =
 				nil, nil, nil, nil, nil, nil
 
-			--Area positions sorting
-			areas.areas[k].pos1, areas.areas[k].pos2 =
-				areas:sortPos(areas.areas[k].pos1, areas.areas[k].pos2)
+			-- Area positions sorting
+			area.pos1, area.pos2 = areas:sortPos(area.pos1, area.pos2)
 
-			--Add name
-			areas.areas[k].name = "unnamed"
+			-- Add name
+			area.name = "unnamed"
+
+			-- Remove ID
+			area.id = nil
 		end
 		minetest.chat_send_player(name, "Table format updated.")
 
@@ -81,9 +82,7 @@ function areas.getNodeOwnerName(pos)
 		if pos.x >= p1.x and pos.x <= p2.x and
 		   pos.y >= p1.y and pos.y <= p2.y and
 		   pos.z >= p1.z and pos.z <= p2.z then
-			if area.owner ~= nil then
-				return area.owner
-			end
+			return area.owner
 		end
 	end
 	return false
@@ -138,6 +137,7 @@ if areas.legacy_table then
 				a.y2 = a.pos2.y
 				a.z2 = a.pos2.z
 				a.pos1, a.pos2 = nil, nil
+				a.id = key
 			end
 			return a
 		end,
@@ -151,10 +151,11 @@ if areas.legacy_table then
 				a.pos2.x = a.x2
 				a.pos2.y = a.y2
 				a.pos2.z = a.z2
-				a.x1, a.y1, a.z1, a.x2, a.y2, a.z2
-				= nil, nil, nil, nil, nil, nil
+				a.x1, a.y1, a.z1, a.x2, a.y2, a.z2 =
+					nil, nil, nil, nil, nil, nil
 				a.name = a.name or "unnamed"
-				return rawset(areas.areas, key, a);
+				a.id = nil
+				return rawset(areas.areas, key, a)
 			end
 		end
 	})
diff --git a/pos.lua b/pos.lua
index 9bf5205..018ee72 100644
--- a/pos.lua
+++ b/pos.lua
@@ -22,19 +22,18 @@ minetest.register_chatcommand("select_area", {
 					"Invalid usage, see /help select_area.")
 			return
 		end
-
-		for k, area in pairs(areas.areas) do
-			if area.id == id then
-				areas:setPos1(name, area.pos1)
-				areas:setPos2(name, area.pos2)
-				minetest.chat_send_player(name,
-						"Area "..id.." selected.")
-				return
-			end
+		if not areas.areas[id] then
+			minetest.chat_send_player(name,
+					"The area "..id.." does not exist.")
+			return
 		end
+
+		areas:setPos1(name, areas.areas[id].pos1)
+		areas:setPos2(name, areas.areas[id].pos2)
 		minetest.chat_send_player(name,
-				"The area "..id.." does not exist.")
-end})
+				"Area "..id.." selected.")
+	end,
+})
 
 minetest.register_chatcommand("area_pos1", {
 	params = "[X Y Z|X,Y,Z]",