Storing Players In a Table review?

In an attempt to practice with tables, I made a script that adds the player that has touched the parent part, as long as they aren’t in the table already and removes them when they leave. Thoughts?

part = script.Parent
mytable = {}
db = {}
allstats = {}
script.Parent.Touched:Connect(function(hit)
	
	local hum = hit.Parent:FindFirstChild("Humanoid") 
	if hum then
		if not table.find(db,hum) then
			table.insert(db,hum)
		end
		if db[hum] == nil or db[hum] == true then
		db[hum] = false
		local player = game:GetService("Players"):GetPlayerFromCharacter(hit.Parent)
		if player then	
			local iterations = 0
			for i,v in pairs (mytable) do
				iterations = iterations + 1
			end
			if iterations == 0 then
				table.insert(mytable,player)
				print("theres no one in the table so just add the player")
				table.insert(allstats,player.Character.Value.Value)
			end
			if not table.find(mytable,player) then
				table.insert(mytable,player)
				table.insert(allstats,player.Character.Value.Value)
			end		
			table.sort(allstats, function()
  				return 2 > 1
			end)
	
			wait(5)
			db[hum] = true
			end
		end
	end
end)

game.Players.PlayerRemoving:Connect(function(plr)
	local position = table.find(mytable,plr)
	local position2 = table.find(db,plr.Character.Humanoid)
	print("getting the players position")
		if position ~= nil then
			print("if their position is not nil, then we remove them")
			table.remove(mytable,position)
			print("removing"..plr.Name)

			end
			if position2 ~= nil then
				table.remove(db,position2)
				print("removing"..plr.Name.."From db table")
		end
end)

1 Like

Not sure what you are asking for here.
Is it the code layout, Use of variables, Area where a mislead may occur or what?

1 Like

I’m asking if its efficient, if there’s any memory leaks, possible errors.

1 Like

Lots of small things that could or should be changed…

The scattered print()

calls are fine when you’re personally debugging the code that you’re in the middle of writing. But they need to be removed from final code or replaced with a dedicated debug print function that can be turned off. The details of how this script works is not relevant during normal gameplay. What happens might be relevant sometimes. At most, it should print “player started touching [name of the touch trigger]” and “player stopped touching [name of the touch trigger]”. Maybe “touch delay for [name of the touch trigger] expired”. Imagine a real game, where you’ll have 10s to 100s times this amount of code. Useful debug messages drowning in all the useless ones means you can’t get the information you actually want. If they’re supposed to be comments, then have them as comments.

Any time you want to check

if var ~= nil then, you can pretty much replace it with if var then. In very few cases is it less readable, and it’s more concise. In most cases, it reads like English, making it more intuitive to read.

Your indentation is all over the place.

E.g. these two LoC:

if db[hum] == nil or db[hum] == true then
db[hum] = false

Indentation is incredibly important for readability, especially when it comes to figuring out which scope different variables belong. For your own sake, and for the sake anyone else reading your code, make sure you do that correctly.

All of

local iterations = 0 
for i,v in pairs (mytable) do iterations = iterations + 1 end
if iterations == 0 then
	...
end

can be replaced with

if #mytable == 0 then
    ...
end

… assuming mytable is an array, which it is assuming that table.insert(mytable,player) is the only place where you modify the contents of mytable. If you really need to walk through the table to count the number of elements, then iterations is a bad variable name. count is a much better variable name. tableLength or tableCount are other decent names. There should also be a comment explaining why you’re doing it in this way.

return 2 > 1

always evaluates to true, and should be changed to just return true. It’s also very unclear what the table.sort call is for. It clearly doesn’t sort the allstats table. I think it might just keep it in the same order? Even if it did sort the table, there’s no indication as to why you would want to do that? It’s certainly not part of the explanation you gave of the purpose of this whole script. If it’s some weird, hacky way of accomplishing something, it needs a comment to explain how and why it does so.

Speaking of variable names,

you need to work on that. The only things I wouldn’t change are the player and hum variables.

Arguably, position is a decent name for holding a table index, but I would make it more verbose to actually describe what it is or what it’s used for. I’d personally change position to playerIndex (and change plr to player), and position2 to dbIndex.

I would rename part to touchTrigger because the classname of the object is less relevant than its purpose. Also, you never actually use that variable.

mytable, db and allstats all need to be changed to actually reflect what their purpose or contents are. Even after reading through the script and understanding their contents at different points in time, I’m not sure what their purpose are. I think db is a dictionary of character’s humanoids to bools, indicating if the character that owns the humanoid is currently considered to be touching the touch trigger? Or maybe it’s a debounce to only let the player “touch” the trigger once every 5 seconds?

I’m assuming player.Character.Value.Value is the Value of e.g. a NumberValue? That ValueObject needs to be renamed to whatever it actually represents. Presumably some kind of stat, since it’s inserted into the allstats table? Impossible to guess.

If you want an example script with all of these style things fixed, please post a detailed description of the this script.

-----

Hope this helps. Keep in mind that anything relating to style is mostly just my opinion, but also that having a style that’s consistent with other people’s is important for readability. Let me know if you have any other questions.

2 Likes

Thanks for the input! :grinning: