Is there a way i can shorten some of this code?

Help and Feedback Code Review
#1

Made a lobby system from scratch and it works, althought alot of the code is pasted and it just doesnt look as good

so im wondering if theres anyway i can cut down on some of this code

-- Varibles
local Players = game:GetService("Players")
local HeldCharacters = game.ReplicatedStorage.HeldCharacters
local CreateLobbyEvent = game.ReplicatedStorage.Events.CreateLobby
local JoinLobbyEvent = game.ReplicatedStorage.Events.JoinLobby
local KickEvent = game.ReplicatedStorage.Events.LobbyKick
local ChangeCamEvent = game.ReplicatedStorage.Events.ChangeCam
local STCLobby = game.ReplicatedStorage.Events.ServerToClientLobby
local DisbandLobbyEvent = game.ReplicatedStorage.Events.DisbandLobby
local leaveLobbyEvent = game.ReplicatedStorage.Events.LeaveLobby
local GiveOwnershipEvent = game.ReplicatedStorage.Events.GiveHost
local StartGameFunction = game.ReplicatedStorage.Events.StartGame
local StarGameEvent = game.ReplicatedStorage.Events.LobbyStartGame
local LobbyMod = require(script.CreateLobby)

local Lobbys = {}
local PlayerThings = {}

-- Functions
local function CheckPlayer(Player)
	repeat task.wait() until PlayerThings[Player]
	
	local PlayerInfo = PlayerThings[Player]
	
	while task.wait(.25) do
		if not Player then break end
		
		local Success, ErrorMessage = pcall(function()
			if PlayerInfo["IsInRoom"] and PlayerInfo["RoomIn"] then
				if not PlayerInfo["RoomIn"]:FindFirstChild("Players") or not PlayerInfo["RoomIn"]:FindFirstChild("Players"):FindFirstChild(Player.Name) then
					PlayerInfo["IsInRoom"] = false
					PlayerInfo['RoomIn'] = nil
					PlayerInfo['CreatedRoom'] = nil
					PlayerInfo["HasCreatedRoom"] = nil
					STCLobby:FireClient(Player, "Leave")
				end
			end
		end)
		
		if not Success then
			warn(ErrorMessage)
			local S, Er = pcall(function()
				PlayerInfo["IsInRoom"] = false
				PlayerInfo['RoomIn'] = nil
				PlayerInfo['CreatedRoom'] = nil
				PlayerInfo["HasCreatedRoom"] = nil
				STCLobby:FireClient(Player, "Leave")
			end)
		end
	end	
end

local function DisbandLobby(Room)
	for i, v in ipairs(Room.Players:GetChildren()) do
		v:Destroy()
	end
	Lobbys[Room] = nil
	Room:Destroy()
end


local function CreateLobby(Player, RoomName, MaxPlayers, Map, IsLocked, Password)
	if not PlayerThings[Player] then return end

	local PlayerInfo = PlayerThings[Player]
	if PlayerInfo["IsInRoom"] == true or PlayerInfo["HasCreatedRoom"] == true then return end
	
	local LobbyTable = LobbyMod.New(Player, RoomName, MaxPlayers, Map, IsLocked, Password)
	
	if not LobbyTable then return end
	
	LobbyTable:StartLobby()
	local Lobby = LobbyTable["Lobby"]
	Lobbys[Lobby] = LobbyTable

	local PlayerValue = Instance.new("StringValue")
	PlayerValue.Name = Player.Name

	PlayerValue.Parent = LobbyTable["PlayerFolder"]
	
	PlayerInfo["IsInRoom"] = true; PlayerInfo["RoomIn"] = LobbyTable["Lobby"]
	PlayerInfo["HasCreatedRoom"] = true; PlayerInfo["CreatedRoom"] = LobbyTable["Lobby"]
	STCLobby:FireClient(Player, "Join", LobbyTable["Lobby"])
end

local function JoinLobby(Player, Room, Password)
	if not PlayerThings[Player] then return end
	
	-- Checking if player can join the room
	local PlayerInfo = PlayerThings[Player]
	if PlayerInfo["IsInRoom"] == true or PlayerInfo["HasCreatedRoom"] == true then return end
	
	if not Lobbys[Room] then return end
	local LobbyInfo = Lobbys[Room]
	
	if table.find(LobbyInfo["BannedUsers"], Player) then return end
	if LobbyInfo["IsLocked"] == true then if LobbyInfo["Password"] ~= Password then return end end
	
	local MaxPlayers = LobbyInfo["MaxPlayers"]
	MaxPlayers = tonumber(MaxPlayers)
	if #LobbyInfo["PlayerFolder"]:GetChildren() >= MaxPlayers then return end
	
	-- Joining the room
	local PlayerValue = Instance.new("StringValue")
	PlayerValue.Name = Player.Name
	PlayerValue.Parent = LobbyInfo["PlayerFolder"]
	PlayerInfo["IsInRoom"] = true; PlayerInfo["RoomIn"] = Room
	STCLobby:FireClient(Player, "Join", Room)
end

local function ForceLeave(PlayerSent, Player, Type)
	if PlayerSent == Player then return end
	if not PlayerThings[PlayerSent] then return end
	local _PInfo = PlayerThings[PlayerSent]
	
	-- Checking If kick / ban is valid
	if not PlayerThings[Player] then return end
	local PlayerInfo = PlayerThings[Player]
	
	if not PlayerInfo["IsInRoom"] == true or not PlayerInfo['RoomIn'] then return end
	local Room = PlayerInfo["RoomIn"]
	
  	-- Big Check for player
	if not _PInfo["HasCreatedRoom"] or not _PInfo["IsInRoom"] or not _PInfo["CreatedRoom"] then return end
	if _PInfo['CreatedRoom'] ~= PlayerInfo["RoomIn"] then return end
	
	-- Kicking Player
	if Type == "Kick" then
		Room:FindFirstChild("Players"):FindFirstChild(Player.Name):Destroy()
		ChangeCamEvent:FireClient(Player, "Menu")
	elseif Type == "Ban" then
		Room:FindFirstChild("Players"):FindFirstChild(Player.Name):Destroy()
		local LobbyInfo = Lobbys[PlayerInfo["RoomIn"]]
		if not table.find(LobbyInfo["BannedUsers"], Player) then
			table.insert(LobbyInfo["BannedUsers"], Player)
		end
	else
		return
	end
end

local function LeaveLobby(Player, Room)
	if not PlayerThings[Player] then return end
	local playerinfo = PlayerThings[Player]

	if not playerinfo["IsInRoom"] and not playerinfo["RoomIn"] then return end
	if not Lobbys[Room] then return end

	Room.Players:FindFirstChild(Player.Name):Destroy()
	
	if Player.Name == Room.LobbyOwner.Value and #Room.Players:GetChildren() > 0 then
		local T = {}
		for i, v in ipairs(Room.Players:GetChildren()) do
			T[i] = v
		end
		if #T > 1 then
			local RandomHost = T[math.random(1,#Room.Players:GetChildren())]
			Room.LobbyOwner.Value = RandomHost.Name

			local Playerd = Players[RandomHost.Name]
			local __Pinfo = PlayerThings[Playerd]
			__Pinfo["HasCreatedRoom"] = true; __Pinfo["CreatedRoom"] = Room
		else
			local Host = T[1]
			Room.LobbyOwner.Value = Host.Name

			local Playerd = Players[Host.Name]
			local __Pinfo = PlayerThings[Playerd]
			__Pinfo["HasCreatedRoom"] = true; __Pinfo["CreatedRoom"] = Room
		end
	end

	if #Room.Players:GetChildren() == 0 then
		DisbandLobby(Room)
	end
end

local function EventDisband(Player, Room)
	if not PlayerThings[Player] then return end
	local _PInfo = PlayerThings[Player]
	
	-- Reused code because im lazy
	if not _PInfo["HasCreatedRoom"] or not _PInfo["IsInRoom"] or not _PInfo["CreatedRoom"] or not _PInfo["CreatedRoom"] == _PInfo["RoomIn"] then return end
	if not _PInfo["CreatedRoom"] == Room and not _PInfo["RoomIn"] == Room then return end
	
	if not Lobbys[Room] then return end
	DisbandLobby(Room)
end

local function GiveHost(Player, PlayerToGiveTo, Room)
	if Player == PlayerToGiveTo then return end
 	-- Giant Check, most of it is pasted because im not writing it out again
	if not PlayerThings[Player] then return end
	local _PInfo = PlayerThings[Player]
	if not _PInfo["HasCreatedRoom"] or not _PInfo["IsInRoom"] or not _PInfo["CreatedRoom"] or not _PInfo["CreatedRoom"] == _PInfo["RoomIn"] then return end
	if not _PInfo["CreatedRoom"] == Room and not _PInfo["RoomIn"] == Room then return end
	if not Lobbys[Room] then return end
	
	if not PlayerThings[PlayerToGiveTo] then return end
	local PlayerInfo = PlayerThings[PlayerToGiveTo]

	if not PlayerInfo["IsInRoom"] == true or not PlayerInfo['RoomIn'] then return end
	local Room = PlayerInfo["RoomIn"]
	
	if _PInfo['CreatedRoom'] ~= PlayerInfo["RoomIn"] then return end
	
	Room.LobbyOwner.Value = PlayerToGiveTo.Name
	PlayerInfo["HasCreatedRoom"] = true; PlayerInfo["CreatedRoom"] = Room
	_PInfo["HasCreatedRoom"] = false; _PInfo["CreatedRoom"] = nil
	
	return "Successful"
end

local function StartGame(Player, Room)
	if not PlayerThings[Player] then return end
	local _PInfo = PlayerThings[Player]
	if not _PInfo["HasCreatedRoom"] or not _PInfo["IsInRoom"] or not _PInfo["CreatedRoom"] or not _PInfo["CreatedRoom"] == _PInfo["RoomIn"] then return end
	if not _PInfo["CreatedRoom"] == Room or not _PInfo["RoomIn"] == Room then return end
	if not Lobbys[Room] then return end
	
	
	local DidItWork = StartGameFunction:Invoke(Room)
	
	repeat task.wait() until DidItWork ~= nil
	
	if not DidItWork then
		local Retries = 2
		local TimesTried = 0
		while wait() do
			TimesTried += 1
			local Diditwork2 = StartGameFunction:Invoke(Room)
			
			repeat task.wait() until DidItWork
			
			if DidItWork then
				break
			end
			
			if TimesTried >= Retries then
				warn("Failed to start game")
				-- Somewhere about firing client it failed
				break
			end
		end
	end
end

-- Event Functions
CreateLobbyEvent.OnServerEvent:Connect(CreateLobby)
JoinLobbyEvent.OnServerEvent:Connect(JoinLobby)
DisbandLobbyEvent.OnServerEvent:Connect(EventDisband)
leaveLobbyEvent.OnServerEvent:Connect(LeaveLobby)
KickEvent.OnServerEvent:Connect(ForceLeave)
GiveOwnershipEvent.OnServerEvent:Connect(GiveHost)
StarGameEvent.OnServerEvent:Connect(StartGame)

Players.PlayerAdded:Connect(function(Player)
	PlayerThings[Player] = {
		IsInRoom = false,
		HasCreatedRoom = false,
	}
	
	local CheckCoroutine = coroutine.create(CheckPlayer)
	coroutine.resume(CheckCoroutine, Player)
end)

Players.PlayerRemoving:Connect(function(Player)
	if PlayerThings[Player] then
		PlayerThings[Player] = nil
	end
end)
2 Likes
#2

You can just use for loop assigning them on a table:

local ReplicatedStorage = game:GetService("ReplicatedStorage"):GetChildren()
local Events= {}

for _, event : Instance in ipairs(ReplicatedStorage.Events:GetChildren()) do
	Events[event.Name] = event
end

Events.StarGameEvent:Fire() -- if its a bindable event.

If you want to check if something exists on the table which is the same as :FindFirstChild() instead just do this:

if (Events.StarGameEvent) then
	continue
end
1 Like
#3

:GetChildren returns an Array. This means you can’t do this I believe:
Events.StarGameEvent:Fire()

instead you can only do this:Events[1]:Fire()

another thing for shortening this though is:

local RS = game:GetService("ReplicatedStorage");
local HeldCharacters = RS.HeldCharacters
local CreateLobbyEvent = RS.Events.CreateLobby
local JoinLobbyEvent = RS.Events.JoinLobby
local KickEvent = RS.Events.LobbyKick
local ChangeCamEvent = RS.Events.ChangeCam
local STCLobby = RS.Events.ServerToClientLobby
local DisbandLobbyEvent = RS.Events.DisbandLobby
local leaveLobbyEvent = RS.Events.LeaveLobby
local GiveOwnershipEvent = RS.Events.GiveHost
local StartGameFunction = RS.Events.StartGame
local StarGameEvent = RS.Events.LobbyStartGame

Also I can see that all of these are events. Have you considered only using 1? You can send an argument from the client specifying the reason for the connection:

:FireServer("LobbyStartGame")

Server:

.OnServerEvent:Connect(function(client, reason)
	if(reason == "LobbyStartGame") then
		--Code here
	end;
end);
2 Likes
#4

Yeah I just notice it just now; that’s why I’m currently editing it.

#5

That depends if the event is being fired multiple times like guns, which is being fired multiple times by players; that can create queues on the remote event/function.

1 Like
#6

Remote events run in parallel. They are separate threads, so there is no queue.

Edit: I tested it and remote functions also run in parallel.

1 Like
#7

Not really queue, I don’t really know to explain this, lets just say avoid doing it but that’s on you; also its messy, having a lot of if statement on a single event; also it check what type of argument was passed, you’ll understand this if you are more about speed/efficiency.

#8

I don’t understand. Can you elaborate on what sort of queue this is? The server only requires 1 HTTP request to then process things. There is the same amount of HTTP requests if you have more events. They are just different.

If I fire 10 bullets, those are 10 requests, it doesn’t matter if it is on a single event or on separate events since it will call the same amount of requests.

And in relation to the if statements. I understand but the reason I prefer to use this is that I believe it is more messy to have more variables for different types.

1 Like
#9

Lets say you have a gun and every player is requesting the same remote event that has a lot of if statement like Fire, Reload, and more not just more but everything that is being fired by the event; it would have to go through all of the if statements until it finds the exact same argument, which cause the server to lag a bit depending on how many if statement there are, something like a million if statement (unlikely to happen) might be noticeable.

1 Like
#10

Also this is alright if its like a gun which only have 2 argument Fire and Reload or inventory Add, Equip and Remove; what I was talking about is using just a single event for everything.

#11

A million? That is a lot!

Either way I made a loop that loops 1 million times with an if statement. The difference is minimal:
image

That is 4 milliseconds.
(The reaason I substracted is because looping itself takes time).

A million if statements is a lot, something more reasonable like 1k (which will probably never be reached for an event) takes about this time:
image
Which is less than a millisecond.

Take note that a for loop also has additional timing which means the time can also be less.

Code:

local startTime = os.clock();
for i=1,1000,1 do
	if(i==1000) then
		print("Yey!");
	end;
end;

local firstTest = os.clock() - startTime;
print("First loop finished in:", firstTest)

startTime = os.clock();
for i=1,1000,1 do
end;

local secondTest = os.clock() - startTime;
print("Second loop finished in:", secondTest)

print("Result:", math.abs(secondTest-firstTest))
#12

Try doing it on a multiple player that’s what I did, also some players have done it as well there was a voluntary players that joined the server to test how laggy will it be.

#13

Wait, I need to correct myself. I figured out how to test how much time a for loop takes:
image

These are the real results which are astonishing!

A million times:
image

The for loop took half of the time. The real time for 1 million if statements are 2 milliseconds.

Code:

local startTime = os.clock();
for i=1,1000000,1 do
	if(i==1000) then
		print("Yey!");
	end;
end;

local firstTest = os.clock() - startTime;

local startTime = os.clock();
for i=1,1000000,1 do
	if(i==999999) then
		print("Yey!");
	elseif(i==1000000) then
		print("Second test!");
	end;
end;

local secondTest = os.clock() - startTime - firstTest;
print("First loop finished in:", secondTest)

startTime = os.clock();
for i=1,1000000,1 do
end;

local thirdTest = os.clock() - startTime;
print("Second loop finished in:", thirdTest)

print("Result:", math.abs(thirdTest-secondTest))

By the way let me test multiplayer

#14

Tested it. This is the difference:

.00000014711559532980363 --In favour of different events

Different events are faster but the number is so small that we need to simplify it using e-7
image

Iterations: 104 (Amount of time called for both events).

The difference was averaged for both getting all the results.

Server code:

local allResults = {};
local multiUseResults = {};

for i=1,4,1 do
	game:GetService("ReplicatedStorage")[i].OnServerEvent:Connect(function(client)
		local startTime = os.clock();
		local result = os.clock() - startTime;
		table.insert(allResults, result);
	end);
end;

game:GetService("ReplicatedStorage"):WaitForChild("multiUse").OnServerEvent:Connect(function(client, reason)
	local startTime = os.clock();
	if(reason == "ReasonOne") then
	elseif(reason == "ReasonTwo") then
	elseif(reason == "ReasonThree") then
	elseif(reason == "ReasonFour") then
	end;
	local result = os.clock() - startTime;
	table.insert(multiUseResults, result);
end);

while #allResults < 101 do
	task.wait(0.1);
	local differentEvents = 0;
	for _, r in pairs(allResults) do
		differentEvents += r;
	end;
	
	differentEvents /= #allResults;
	

	local sameEvent = 0;
	for _, r in pairs(multiUseResults) do
		sameEvent += r;
	end;

	sameEvent /= #multiUseResults;
	
	print("The current difference is:", sameEvent - differentEvents);
	warn("Iterations:", #allResults);
end;

Client code:

local reasons = {
	"ReasonOne",
	"ReasonTwo",
	"ReasonThree",
	"ReasonFour"
};

coroutine.wrap(function()
	while true do
		task.wait(0.01);
		game:GetService("ReplicatedStorage")[math.random(1,4)]:FireServer();
	end;
end)();

local multUse = game:GetService("ReplicatedStorage"):WaitForChild("multiUse");
while true do
	task.wait(0.01);
	multUse:FireServer(reasons[math.random(1,4)]);
end;

Location of Remotes:
image

#15

Here the test should be something like this (also I made as a plugin so it wouldn’t execution time):

-- I lowered it to just ten thousand because it took more than a minute
Result: 72771ms or 72.771s

local toolbar = plugin:CreateToolbar("Test")
local Test = toolbar:CreateButton("Test", "Test", "rbxassetid://4458901886")

Test.ClickableWhenViewportHidden = true

local function doSomething(index : number)
	if (typeof(index) == "number") then
		local x = (index - index)
		
		if (x == 0) then
			local y = (index - (index - math.pi))
			
			if (y == -math.pi) then
				return
			end
		end
	end
end

local function checkIfIndex(argument : number)
	for index = 1, 100000 do
		if (argument == index) then
			coroutine.wrap(doSomething)(argument)
		end
	end
end


local function StartTest()
	local start = tick()
	
	for argument = 1, 100000 do
		-- Remote event does not yield
		-- This is the perspective of a remote event
		coroutine.wrap(checkIfIndex)()
	end

	-- In reality the result should be 0.001,
	-- but it's not because the server is lagging

	local result = (tick() - start)

	print(string.format("Result: %ims or %3f", math.round(result * 1000), result))
end

Test.Click:Connect(StartTest)
#17

So something like

local Events = {}
for i, v in ipairs(game.ReplicatedStorage.Events:GetChildren()) do
	Events[v.Name] = v
end

-- Then something like
Events["StartLobby"].OnServerEvent:Connect(function() print(1) end)

Perhaps, but id have to change nearly the entire script

#18

Events["StartLobby"]. this is unnecessary just use Events.StartLobby, if you understand this correctly almost everything is a table, but some of them are un-writable like Enum, in C you can add enum {test, something, etc.} on roblox we use tables instead of Enum to compare something that works like an enum we use metatables.

You can just find and replace.
Screenshot_6

1 Like
#19

id have to remove all the functions, and put it all in one function and just have alot of elseifs and stuff, plus id have to change the way the events fire completely, adding a new variable at #2 to detect what “elseif” it should go to.

or i can skip step one and keep the functions, but id still have to add alot of elseifs to detect what functon to fire, either way it doesnt really help that much

#20

Avoid doing this especially if there are multiple if statements its messy and unorganized to look at; just stick to readable/understandable functions this makes it easier to understand the code; if ever you want to ask for help other players would be able to understand easily. Also if you want to know what unorganized code looks like, is having comments to make it understandable (in my opinion).