Reptitive Code (I broke DRY)

[dzhann757 (Apollo)]

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

local Sword_Fight_Arena = workspace:WaitForChild("Sword_Fight_One")

local Arena_Values = Sword_Fight_Arena:WaitForChild("Arena_Player_Values")
local Player_One_ID = Arena_Values:WaitForChild("Player_One_ID")
local Player_Two_ID = Arena_Values:WaitForChild("Player_Two_ID")
local Players_In_Arena = Arena_Values:WaitForChild("Players_In_Arena")

local Join_Parts = Sword_Fight_Arena:WaitForChild("Join_Parts")
local Join_Part_Player_One = Join_Parts:WaitForChild("Join_Player_One")
local Join_Part_Player_Two = Join_Parts:WaitForChild("Join_Player_Two")

local Player_One_Prompt = Join_Part_Player_One.Attachment:WaitForChild("Player_One_Prompt")
local Player_Two_Prompt = Join_Part_Player_Two.Attachment:WaitForChild("Player_Two_Prompt")

local Lobbies = workspace:WaitForChild("Lobbies")
local Secondary_Lobby = Lobbies:WaitForChild("Secondary_Lobby")
local Lobby = Secondary_Lobby:WaitForChild("lobby")
local Secondary_Spawn = Lobby:WaitForChild("Secondary_Spawn")

local Modules = ReplicatedStorage:WaitForChild("Modules")
local Arena_Modules = Modules:WaitForChild("Arena_Modules")
local Arena_Functions = require(Arena_Modules:WaitForChild("Arena_Functions"))

local Player_One_Color = Color3.fromRGB(255, 0, 0)
local Player_Two_Color = Color3.fromRGB(0, 100, 255)

Players.PlayerAdded:Connect(function(player)
	local Player_ID = player.UserId
	
	player.CharacterAdded:Connect(function(character)
		local Humanoid = character:WaitForChild("Humanoid")
		
		Humanoid.Died:Once(function()
			if Player_ID == Player_One_ID.Value then
				Player_One_ID.Value = 0
				Players_In_Arena.Value = 0
				
				if Player_Two_ID.Value == 0 then
					print("Player One resetted and there was no one in Player Two.")
					
					Arena_Functions.Toggle_Join_Prompt_and_Part(Join_Part_Player_One, Player_One_Prompt, true, Player_One_Color)
					-- This module function just tweens the join part color back to normal and enables the Player Prompt
					return
				end
				
				local Opponent = Players:GetPlayerByUserId(Player_Two_ID.Value)
				local Opponent_Character = Opponent.Character
				
				Arena_Functions.Create_ForceField(Opponent_Character)
				-- Creates forcefield around the opponent's character so they don't get spawn killed when they are teleported back
				Arena_Functions.Award_Winner_Leaderstats(Players:GetPlayerByUserId(Opponent))
				-- Increases the Opponent's leaderstats
				Player_Two_ID.Value = 0
				
				Opponent_Character:PivotTo(CFrame.new(Secondary_Spawn.Position + Vector3.new(0, 5, 0)))
				
				task.spawn(Arena_Functions.Toggle_Join_Prompt_and_Part, Join_Part_Player_One, Player_One_Prompt, true, Player_One_Color)
				task.spawn(Arena_Functions.Toggle_Join_Prompt_and_Part, Join_Part_Player_Two, Player_Two_Prompt, true, Player_Two_Color)
				
			elseif Player_ID == Player_Two_ID.Value then
				Player_Two_ID.Value = 0
				Players_In_Arena.Value = 0
				
				if Player_One_ID.Value == 0 then
					print("Player Two resetted and there was no one in Player One.")

					Arena_Functions.Toggle_Join_Prompt_and_Part(Join_Part_Player_Two, Player_Two_Prompt, true, Player_Two_Color)
					return
				end
				
				local Opponent = Players:GetPlayerByUserId(Player_One_ID.Value)
				local Opponent_Character = Opponent.Character

				Arena_Functions.Create_ForceField(Opponent_Character)
				Arena_Functions.Award_Winner_Leaderstats(Players:GetPlayerByUserId(Opponent))

				Opponent_Character:PivotTo(CFrame.new(Secondary_Spawn.Position + Vector3.new(0, 5, 0)))
				Player_One_ID.Value = 0

				task.spawn(Arena_Functions.Toggle_Join_Prompt_and_Part, Join_Part_Player_One, Player_One_Prompt, true, Player_One_Color)
				task.spawn(Arena_Functions.Toggle_Join_Prompt_and_Part, Join_Part_Player_Two, Player_Two_Prompt, true, Player_Two_Color)
			end
		end)
	end)
end)

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

local Sword_Fight_Arena = workspace:WaitForChild("Sword_Fight_One")

local Arena_Values = Sword_Fight_Arena:WaitForChild("Arena_Player_Values")
local Player_One_ID = Arena_Values:WaitForChild("Player_One_ID")
local Player_Two_ID = Arena_Values:WaitForChild("Player_Two_ID")
local Players_In_Arena = Arena_Values:WaitForChild("Players_In_Arena")

local Join_Parts = Sword_Fight_Arena:WaitForChild("Join_Parts")
local Join_Part_Player_One = Join_Parts:WaitForChild("Join_Player_One")
local Join_Part_Player_Two = Join_Parts:WaitForChild("Join_Player_Two")

local Player_One_Prompt = Join_Part_Player_One.Attachment:WaitForChild("Player_One_Prompt")
local Player_Two_Prompt = Join_Part_Player_Two.Attachment:WaitForChild("Player_Two_Prompt")

local Lobbies = workspace:WaitForChild("Lobbies")
local Secondary_Lobby = Lobbies:WaitForChild("Secondary_Lobby")
local Lobby = Secondary_Lobby:WaitForChild("lobby")
local Secondary_Spawn = Lobby:WaitForChild("Secondary_Spawn")

local Modules = ReplicatedStorage:WaitForChild("Modules")
local Arena_Modules = Modules:WaitForChild("Arena_Modules")
local Arena_Functions = require(Arena_Modules:WaitForChild("Arena_Functions"))

local Player_One_Color = Color3.fromRGB(255, 0, 0)
local Player_Two_Color = Color3.fromRGB(0, 100, 255)

Players.PlayerRemoving:Connect(function(player)
	local Player_ID = player.UserId
	
	if Player_ID == Player_One_ID.Value then
		Player_One_ID.Value = 0
		Players_In_Arena.Value = 0
		
		if Player_Two_ID.Value == 0 then
			print("Player One left and there is no one in Player Two.")
			
			Arena_Functions.Toggle_Join_Prompt_and_Part(Join_Part_Player_One, Player_One_Prompt, true, Player_One_Color)
			return
		end
		
		local Opponent = Players:GetPlayerByUserId(Player_Two_ID.Value)
		local Opponent_Character = Opponent.Character
		
		Arena_Functions.Create_ForceField(Opponent_Character)
		Arena_Functions.Award_Winner_Leaderstats(Opponent)
		
		Opponent_Character:PivotTo(CFrame.new(Secondary_Spawn.Position + Vector3.new(0, 5, 0)))
		Player_Two_ID.Value = 0
		
		task.spawn(Arena_Functions.Toggle_Join_Prompt_and_Part, Join_Part_Player_One, Player_One_Prompt, true, Player_One_Color)
		task.spawn(Arena_Functions.Toggle_Join_Prompt_and_Part, Join_Part_Player_Two, Player_Two_Prompt, true, Player_Two_Color)
		
	elseif Player_ID == Player_Two_ID.Value then
		Player_Two_ID.Value = 0
		Players_In_Arena.Value = 0
		
		if Player_One_ID.Value == 0 then
			print("Player Two left and there is no one in Player One.")
			
			Arena_Functions.Toggle_Join_Prompt_and_Part(Join_Part_Player_Two, Player_Two_Prompt, true, Player_Two_Color)
			return
		end
		
		local Opponent = Players:GetPlayerByUserId(Player_One_ID.Value)
		local Opponent_Character = Opponent.Character

		Arena_Functions.Create_ForceField(Opponent_Character)
		Arena_Functions.Award_Winner_Leaderstats(Opponent)

		Opponent_Character:PivotTo(CFrame.new(Secondary_Spawn.Position + Vector3.new(0, 5, 0)))
		Player_Two_ID.Value = 0

		task.spawn(Arena_Functions.Toggle_Join_Prompt_and_Part, Join_Part_Player_One, Player_One_Prompt, true, Player_One_Color)
		task.spawn(Arena_Functions.Toggle_Join_Prompt_and_Part, Join_Part_Player_Two, Player_Two_Prompt, true, Player_Two_Color)
	end
end)

Hello! I’m making a sword fighting arena where two players will enter and fight. I made two scripts that handles if a player dies and checks if they are a fighter in the arena and when they leave as well. However, I think that my code is unoptimized and unefficient because it is repetitve. I’m trying to use module scripts to reduce repeating my code over and over but I don’t know how since it still looks the same. Any suggestions to improve my code? Also at some point, I will want multiple arenas and I don’t want to keep cloning the same script over and over again.

[ig1oo1 (hello)]

I haven’t thought over it too much yet—perhaps I may come back tomorrow to give it some more thought, but module scripts ARE awesome; however, sometimes, it could be more beneficial to organize chunks of code into functions rather than in modules. Modules may be overkill and overcomplicate things for simpler pieces of code.

Additionally, I notice that most of the code is filled with variable definitions and instance references. It should be fine to skip some variables, and I think you may be overusing :WaitForChild - especially instances like models and folders.

[dzhann757 (Apollo)]

To be honest, I always use :WaitForChild() a lot to ensure that the script waits for the instance to load first before executing code. I always worry about just doing Thing1.Thing1_Child or Thing1:FindFirstChild("Thing1_Child_Name") in case if it doesn’t load in time.

[ig1oo1 (hello)]

You’re absolutely right—I have similar tendencies too. The entire server breaks if just one thing doesn’t load on time. I’m not exactly sure what you can do other than cut down on your variables. Sorry :(

[dzhann757 (Apollo)]

it’s alright lol but not really sure if I should cut them down (I probably should though)

[EffBeeCee]

@dzhann757

To be honest, I always use :WaitForChild() a lot to ensure that the script waits for the instance to load first before executing code. I always worry about just doing Thing1.Thing1_Child or Thing1:FindFirstChild("Thing1_Child_Name") in case if it doesn’t load in time.

@ig1oo1

You’re absolutely right—I have similar tendencies too. The entire server breaks if just one thing doesn’t load on time. I’m not exactly sure what you can do other than cut down on your variables. Sorry :(

This really isn’t a great idea to mainly use :WaitForChild(). Usually, you don’t have to worry about a lot of things having to stream/load in and you should be fine with just using :FindFirstChild(). I’m pretty sure that :WaitForChild() is somewhere around 30% slower than :FindFirstChild().

For example, if you are trying to index a NumberValue that is parented to the script you are working on, you will almost ALWAYS have that instantly available to the script and you can use:

1. . notation - fastest performance but will error if the NumberValue doesn’t exist and can cause issues if you have a property with the same name
2. :FindFirstChild() - faster performance than :WaitForChild() and will work exactly the same as :WaitForChild()

Looking at your post you seem to have way too many WaitForChilds and most of those you can probably replace with :FindFirstChild(). Also, usually people use camelCase for variables, not Snake_Case but it’s kinda up to personal preference.

[dzhann757 (Apollo)]

I mostly worry about things like a model and its children. I put :WaitForChild() for models to make sure it loads and then put :WaitForChild() on its instances too ;-;

[EffBeeCee]

Waiting for models is a valid use of :WaitForChild() since it’s common for models to load after the script loads. The instances under the model should be loaded so you can probably use :FindFirstChild() unless you have a script creating the children of the model.

There’s a high chance you don’t need any WaitForChild()s in your script but I think it would be okay to use it for indexing models.

[dzhann757 (Apollo)]

How do I reduce my code though or is it already good? I’m trying to not repeat

[dzhann757 (Apollo)]

Or just make it more efficient / optimized

[EffBeeCee]

Well, first of all, I don’t see any reason for these to be 2 separate scripts if you can just put the :PlayerAdded and PlayerRemoving in the same script.

Within this elseif statement you have almost the exact same thing running twice. If you just make a function and put some variables as parameters that should be able to cut down on a lot of the code too.

[notmrbeast192011 (Haze)]

Personally, I think you should try to use module for reusable functions

[dzhann757 (Apollo)]

I am already using module scripts for reusable functions.

[notmrbeast192011 (Haze)]

@dzhann757

local Arena_Functions = require(Arena_Modules:WaitForChild("Arena_Functions"))

mb, I didn’t notice that
Overall, your script is good. You don’t have to worry about optimization.
There’s one thing I’m not sure is overusing if statements might take more performance or not.
I’m sorry if my English is terrible.

[dzhann757 (Apollo)]

Oh, it’s alright. Anyways, I’m also not sure if if statements will take up performance?

[notmrbeast192011 (Haze)]

yes I used to avoid if statement

How do I avoid continuously using if statements

One method you could use is replacing the Events table with a dictionary that stores functions for each different type of event, then indexing the table with the event: local Events = { Event1 = function() -- Do something print("Hello!") end, Event2 = function() -- Do something else print("Goodbye!") end, } local event = "Event1" Events[event]() -- Hello! event = "Event2" Events[event]() -- Goodbye! I wish something that works like C++'s switch was possible in Luau though, it w…

In some cases, it’s okay to use if statement like checking if object is existing in game or not.

[dzhann757 (Apollo)]

I’m repeatedly using if statements to check if a fighter in the arena died or left so I can reward the opponent.

[EffBeeCee]

A good way that I think you may want to use to simplify the length of this elseif statement is to use a central function while turning your variables into tables.

local Player_IDs = { -- Use a table to store Player_One_ID and Player_Two_ID together
	Arena_Values:FindFirstChild("Player_One_ID"),
	Arena_Values:FindFirstChild("Player_Two_ID")
}
-- Make other variables with player 1 and 2 into a table...

local function PlayerRemoving(playerNum: number)
	Player_IDs[playerNum].Value = 0 -- Access the variable without needing multiple functions
	Players_In_Arena.Value = 0
	-- Continue following this format...
end

Players.PlayerRemoving:Connect(function(player)
	local Player_ID = player.UserId
	
	if Player_ID == Player_IDs[1].Value then -- Way less code!
		PlayerRemoving(1)
	elseif Player_ID == Player_IDs[2]_ID.Value then
		PlayerRemoving(2)
	end
end)

This is a very short example of what you could do to simplify your code. Following this will improve debugging and make your code easier to read.

@dzhann757

Oh, it’s alright. Anyways, I’m also not sure if if statements will take up performance?

Your code is already quite optimized (after you remove your :WaitForChild calls), you really only need to improve a bit on readability.

[dzhann757 (Apollo)]

Wait, I thought of something. Should I replace the int values (Player One and Two ID and Players_In_Arena) into Attributes?

[notmrbeast192011 (Haze)]

@dzhann757

I will want multiple arenas and I don’t want to keep cloning the same script over and over again.

CollectionService will help you deal with it.
https://create.roblox.com/docs/reference/engine/classes/CollectionService