Multiple gamemodes in individual modules

Help and Feedback Code Review
#1

I’m wanting to get thoughts/opinions on this setup for my gamemodes module. Basically, each gamemode has their own module, a Start and Cleanup function.

Start basically starts and controls any actions that are done in a game. In this example, Team Deathmatch checks for any deaths, and adds points to specific team when player dies.

I just wanna know if this is efficient, is gonna lead to any memory leaks or anything, etc.

local TDM = {}

local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Teams = game:GetService("Teams")

local ScoreManager = require(script.Parent.Parent.ScoreManager)

local Shared = ReplicatedStorage:WaitForChild("Shared")
local Maid = require(Shared.Maid)

local Blue = Teams:WaitForChild("Blue")
local Red = Teams:WaitForChild("Red")

local Connections = {}
local Debounces = {}

--// Start the game
function TDM.Start()	
	for _, player in pairs(Players:GetPlayers()) do
		local Character = player.Character
		if not Character then continue end
		
		Connections[#Connections + 1] = Character.Humanoid.Died:Connect(function(character)
			if Debounces[player.UserId] then return end
			
			Debounces[player.UserId] = true
			
			local Creator = Character:FindFirstChild("Creator")
			if not Creator then return end -- No killer
			
			if player.Team == Blue then
				ScoreManager.Add(Red)
			elseif player.Team == Red then
				ScoreManager.Add(Blue)
			end
		end)
		
		Connections[#Connections + 1] = player.CharacterAdded:Connect(function(character)
			if not Debounces[player.UserId] then return end
			
			Debounces[player.UserId] = nil
		end)
	end
end

--// Do any cleaning up
function TDM.Cleanup()
	for i = 1, #Connections do
		Connections[i]:Disconnect()
		Connections[i] = nil
	end
	
	Debounces = {}
end

return TDM
2 Likes
#2

Overall, your code looks great! It’s simple and gets the job done efficiently. Most of the critiques I have for you are good practice and don’t affect performance by a significant margin.

Maids are pretty much useless imo. You are barely getting any extra functionality or abstraction from this module which is why I recommend not requiring them.

When iterating through tables, you should be using ipairs for arrays and pairs for dictionaries. It gives more context to the developer of what type of table you are iterating through. Not only that, it’s also a slight optimization. “BuT c’MoN dArTh, iT’s JuSt A mIcRo-OpTiMiZaTiOn”. Well you are correct, most of the time micro-optimizing is a waste of time, but that depends on how long it takes to implement the optimization. In this scenario, all it takes is just an extra letter ‘i’ before pairs.

There not really a valid reason to use Connections[#Connections + 1] over table.insert. table.insert is more readable imo, but yet again readability is subjective so if you find that more readable, than ignore my comment. If you are doing this for optimization purposes just note, the luau VM has been re-optimized recently, so table.insert became quicker than manually inserting.

There is no purpose in storing the UserId of the player in the table. The player is an instance. All instances are objects and therefore have an address. Therefore, Debounce[Player] = true will work as intended.

Also, here’s a cool trick :)). You can use short-circuit evaluation here.

ScoreManager.Add(( Player.Team == Blue and Red ) or Blue)

So basically if the player’s team is blue, the ‘and’ operator will be satisfied meaning the result of the expression will be ScoreManager.Add(Red). Otherwise, if the player’s team is not blue, the ‘or’ operator will be satisfied meaning the result of the expression will be ScoreManager.Add(Blue).

You can read more about boolean algebra here:
Boolean Algebra

If you are iterating through an array normally (without intervals such as for i = 1, 6, 2 do end, etc…) then you should be utilizing ipairs. It prevents having to index the value manually. It’s also good practice.

for _, Connection in ipairs(Connections) do
    Connection:Disconnect()
end

If you want to get rid of all the keys of a table, but still want to re-use the table, then I highly discourage doing this. table.clear is a lot more memory efficient way of clearing a table rather than setting the variable to a new table address.

table.clear(Debounces)

Table Documentation

7 Likes
#3

I won’t repeat anything that DarthFS said, but I just want to add a different voice to the conversation about Maids - I think they’re great and your situation is a perfect use case.

Other than that, the main thing I’d change is to not use the “debounce pattern”, in fact I never use it. I only ever see it used to express logic that has nothing to do with debouncing, as is the case here. So if it’s not related to the thing it’s called, it’s just lazy variable naming. Just calling it DeadPlayers instead of Debounce already makes the code a lot more readable, but it can be improved further. Since it’s never accessed outside of for loop scope of the specific player, you can make three improvements with one change: getting rid of global state, getting rid of unnecessary cleanup logic and getting the state closer to where it’s used:

local isDead = false

Maid:GiveTask(character.Humanoid.Died:Connect(function(character)
    if isDead then return end
    isDead = true
    
    ...
end))

Maid:GiveTask(player.CharacterAdded:Connect(function(character)
    isDead = false
end))

local Creator = character:FindFirstChild("Creator")
if not Creator then return end -- No killer

This is a pattern I’ve seen in a lot of free models, and it’s another example of bad variable naming. Just call it Killer instead and get rid of the comment:

local Killer = character:FindFirstChild("Killer")
if not Killer then return end

good on you for noticing that it’s a bad variable name and adding the comment though, but try to fix the problem at the source if you have access to the system that sets up the “Creator” objects.

Here’s how I would change the entire script:

Summary 

local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Teams = game:GetService("Teams")

local ScoreManager = require(script.Parent.Parent.ScoreManager)
local Maid = require(ReplicatedStorage:WaitForChild("Shared").Maid)

local blueTeam = Teams:WaitForChild("Blue")
local redTeam = Teams:WaitForChild("Red")

local TDM = {}

local roundMaid

function TDM.Begin()
	if roundMaid then error("It doesn't make sense to start a round if one is already in progress - call TDM.End first!") end
	roundMaid = Maid.new()

	--Setup scoring for the opposite team when a player is killed
	for _, player in pairs(Players:GetPlayers()) do
		local character = player.Character
		if not character then continue end
		
		local isDead = false
		
		Maid:GiveTask(character.Humanoid.Died:Connect(function(character)
			if isDead then return end
			isDead = true
			
			local Killer = character:FindFirstChild("Killer")
			if not Killer then return end
			
			if player.Team == blueTeam then
				ScoreManager.Add(redTeam)
			elseif player.Team == redTeam then
				ScoreManager.Add(blueTeam)
			end
		end))
		
		Maid:GiveTask(player.CharacterAdded:Connect(function(character)
			isDead = false
		end))
	end
end

function TDM.End()
	roundMaid:DoCleaning()
	roundMaid = nil
end

return TDM

There also appears to be a bug in your code - are players only supposed to give points to the opposite team the first time they die? Because that’s how it’s set up right now.

Anyway, I should probably have read your post closer xD Yes, your original code is efficient and doesn’t cause memory leaks.

1 Like