Very messy round system

Help and Feedback Code Review
#1

Hi,

My round system is very messy and long. I have a feeling it can be shortened drastically. I need help making this more readable:

--> services
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ServerScriptService = game:GetService("ServerScriptService")
local SoundService = game:GetService("SoundService")

--> folders
local Source = ReplicatedStorage.Source
local Dependencies = Source.Dependencies
local ServerDependencies = ServerScriptService.Source.Dependencies
local Assets = ReplicatedStorage.Assets
local Background = SoundService.Master.Background
--> requires
local Settings = require(Dependencies.GameSettings)
local BridgeNet2 = require(Dependencies.BridgeNet2)
local DataManager = require(ServerDependencies.Data)
local Knit = require(ReplicatedStorage.Packages.Knit)
local PlaySound = require(Source.Util.playSound)
--> variables
local Values = Source.Values
--> remotes
local UpdateStats = BridgeNet2.ReferenceBridge("UpdateStats")
--> tables
local Participants = {}
local Data = {}
local GameService = Knit.CreateService{
	Name = "GameService",
	Client = {},
};

--> constants
local isStart = nil 
local isBreak = nil
local previousSeeker = nil

local function timeFormat(s)
	return string.format("%02i:%02i", s/60, s%60)
end

function updateData()
	for _, p in pairs(Players:GetPlayers()) do
		local playerData = DataManager:GetData(p)

		if Data[p.Name] then
			continue
		end

		Data[p.Name] = playerData
	end

	UpdateStats:Fire(BridgeNet2.AllPlayers(), Data)
end

function onDeadPlayerAdded(player: Player)
	table.insert(Participants, player)

	updateData()
	-- isStart = intermission started
	if isStart then
		return
	end

	if #Participants >= Settings.MinimumPlayers then
		isBreak = false

		-- intermission
		for seconds = Settings.Intermission, 1, -1 do
			if isBreak then
				Values:SetAttribute("Status", Settings.MinimumPlayersStatus) 
				isBreak = true
				isStart = false
				break
			end

			Values:SetAttribute("Status", "starting in: "..seconds.." seconds")
			isStart = true

			task.wait(1)
		end

		if isBreak then -- not enough players to start
			isStart = false
			return
		end

		-- set began = handles maps in another module.
		Values:SetAttribute("Began", true) 
		-- small delay before teaming and teleporting
		task.wait(3)

		local map = Assets.Maps[Values:GetAttribute("Map")]
		-- 1-3 = 1 seekers, 4-6 = 2 seekers, 7-9 = 3 seekers.
		local seekerCount = math.ceil(#Participants / 3)

		-- handle spawning
		local RedSpawns = map:FindFirstChild("RedSpawns"):GetChildren()
		local GreenSpawns = map:FindFirstChild("GreenSpawns"):GetChildren()

		local randomRedSpawn = RedSpawns[math.random(1,#RedSpawns)]
		local randomGreenSpawn = GreenSpawns[math.random(1,#GreenSpawns)]

		-- choose seeker(s).
		for i = seekerCount, 1, -1 do
			local random = math.random(1, #Participants)
			local playerPicked = Participants[random]

			if seekerCount == 1 then -- variety in small servers.
				if previousSeeker and playerPicked == previousSeeker then
					repeat 
						random = math.random(1, #Participants)
						playerPicked = Participants[random]
					until playerPicked ~= previousSeeker
				end

				previousSeeker = playerPicked
			end

			playerPicked.Team = Settings.Red
			
			local humanoid = playerPicked.Humanoid
			humanoid.HealthDisplayDistance = Settings.RunnerNameDistance
			humanoid.NameDisplayDistance = Settings.RunnerNameDistance
			
			local root = playerPicked.Character.HumanoidRootPart
			root.CFrame = randomRedSpawn.CFrame + Vector3.new(math.random(1,3),math.random(1,3),math.random(1,3))

			table.remove(Participants, random)
		end

		-- put rest in runners.
		for _, player: Player in pairs(Participants) do
			player.Team = Settings.Green

			local root = player.Character.HumanoidRootPart
			root.CFrame = randomGreenSpawn.CFrame + Vector3.new(math.random(1,3),math.random(1,3),math.random(1,3))

			local humanoid = player.Character.Humanoid :: Humanoid
			
			humanoid.Died:Connect(function()
				player.Team = Settings.Dead
			end)
		end

		local function win()
			-- sfx
			PlaySound(Background.Victory)
			PlaySound(Background.Applause)

			-- delay before winner gets teleported to spawn.
			task.wait(Background.Victory.TimeLength + 1)

			Values:SetAttribute("Began", false) 
			isStart = false

			for _, player in next, Players:GetPlayers() do
				player.Team = nil -- makes it so you need to press play to participate again.

				if player.Character then
					local humanoid = player.Character.Humanoid
					local root = player.Character.HumanoidRootPart
					root.CFrame = workspace.Lobby.SpawnLocation.CFrame * CFrame.new(0,10,0)

					humanoid.HealthDisplayDistance = 50
					humanoid.NameDisplayDistance = 50
				end
			end
		end

		-- tables stay the same
		local runners = Settings.Green:GetPlayers()
		local seekers = Settings.Red:GetPlayers()

		-- game start
		for seconds = Settings.TimeLength, 1, -1 do
			local time = timeFormat(seconds)
			Values:SetAttribute("Status", time)

			-- updated tables
			local greenCount = Settings.Green:GetPlayers()
			local redCount = Settings.Red:GetPlayers()

			if #greenCount == 0 then -- seekers win / all runners dead
				Values:SetAttribute("Status", Settings.SeekerWinStatus)
				for _, runner: Player in pairs(runners) do
					local runnerData = DataManager:GetData(runner)

					if not runnerData or not runnerData.Statistics then -- player left the game
						continue
					end

					runnerData.Statistics.Loses += 1
				end

				for _, seeker: Player in pairs(seekers) do
					local seekerData = DataManager:GetData(seeker)

					if not seekerData or not seekerData.Statistics then 
						continue
					end

					seekerData.Statistics.Wins += 1
					seekerData.Statistics.Coins += Settings.SeekerWinCoins
				end

				win()

				break
			end

			if seconds == 0 or #redCount == 0 then -- runners win / game ended
				Values:SetAttribute("Status", Settings.RunnerWinStatus)

				for _, runner: Player in pairs(greenCount) do
					local runnerData = DataManager:GetData(runner)

					if not runnerData or not runnerData.Statistics then -- player left/  +1 lose regardless
						continue
					end

					runnerData.Statistics.Wins += 1
					runnerData.Statistics.Coins += Settings.RunnerWinCoins
				end

				for _, seeker: Player in pairs(redCount) do
					local seekerData = DataManager:GetData(seeker)

					if not seekerData or not seekerData.Statistics then  -- player left / +1 lose regard
						continue
					end

					seekerData.Statistics.Loses += 1
				end

				win()

				if #redCount == 0 then -- seeker left, end game.
					break
				end
			end

			task.wait(1)
		end
	end
end

function onDeadPlayerRemoved(player: Player)
	--/ remove player from queue.
	if table.find(Participants, player) then
		table.remove(Participants, table.find(Participants, player))
	end

	--/ if participants less than minimum after leave, stop intermission.
	if #Participants < Settings.MinimumPlayers and not Values:GetAttribute("Began") then
		isBreak = true
		Values:SetAttribute("Status", Settings.MinimumPlayersStatus)
	end
end

function gamePresync()
	Settings.Dead.PlayerAdded:Connect(onDeadPlayerAdded)
	Settings.Dead.PlayerRemoved:Connect(onDeadPlayerRemoved)
end

function GameService.KnitStart()
	gamePresync()
end

return GameService
#2

You should probably begin to break down the code into different segments. I’d start with the different functions, and what they do. Under each function, add a comment to highlight exactly what it does.

Also, I’ve noticed there’s a lot of unnecessary stuff, like what is BridgeNet2 supposed to do? Why do you have so many require()'s?

I doubt you’d be able to shorten it without a full rewrite of the code.

1 Like
#3

True, I was removing things with bridgenet2 (networking module) and I forgot to remove the require variable. I’ll try to fully rewrite the code.

(Nvm I used it to update statistics in the game)

#4

Alright so, I made it more readable. It’s still messy but atleast its way easier to read now. I made two new functions: onDeadPlayerAdded, onDeadPlayerRemoved

Every variable is necessary for it to work (I think)

--> services
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ServerScriptService = game:GetService("ServerScriptService")
local SoundService = game:GetService("SoundService")

--> folders
local Source = ReplicatedStorage.Source
local Dependencies = Source.Dependencies
local ServerDependencies = ServerScriptService.Source.Dependencies
local Assets = ReplicatedStorage.Assets
local Background = SoundService.Master.Background
--> requires
local Settings = require(Dependencies.GameSettings)
local BridgeNet2 = require(Dependencies.BridgeNet2)
local DataManager = require(ServerDependencies.Data)
local Knit = require(ReplicatedStorage.Packages.Knit)
local PlaySound = require(Source.Util.playSound)
--> variables
local Values = Source.Values
--> remotes
local UpdateStats = BridgeNet2.ReferenceBridge("UpdateStats")
--> tables
local Participants = {}
local Data = {}
local GameService = Knit.CreateService{
	Name = "GameService",
	Client = {},
};

--> constants
local isStart = nil 
local isBreak = nil
local previousSeeker = nil

local function timeFormat(s)
	return string.format("%02i:%02i", s/60, s%60)
end

function updateData()
	for _, p in pairs(Players:GetPlayers()) do
		local playerData = DataManager:GetData(p)

		if Data[p.Name] then
			continue
		end

		Data[p.Name] = playerData
	end

	UpdateStats:Fire(BridgeNet2.AllPlayers(), Data)
end

function onDeadPlayerAdded(player: Player)
	table.insert(Participants, player)

	updateData()
	-- isStart = intermission started
	if isStart then
		return
	end

	if #Participants >= Settings.MinimumPlayers then
		isBreak = false

		-- intermission
		for seconds = Settings.Intermission, 1, -1 do
			if isBreak then
				Values:SetAttribute("Status", Settings.MinimumPlayersStatus) 
				isBreak = true
				isStart = false
				break
			end

			Values:SetAttribute("Status", "starting in: "..seconds.." seconds")
			isStart = true

			task.wait(1)
		end

		if isBreak then -- not enough players to start
			isStart = false
			return
		end

		-- set began = handles maps in another module.
		Values:SetAttribute("Began", true) 
		-- small delay before teaming and teleporting
		task.wait(3)

		local map = Assets.Maps[Values:GetAttribute("Map")]
		-- 1-3 = 1 seekers, 4-6 = 2 seekers, 7-9 = 3 seekers.
		local seekerCount = math.ceil(#Participants / 3)

		-- handle spawning
		local RedSpawns = map:FindFirstChild("RedSpawns"):GetChildren()
		local GreenSpawns = map:FindFirstChild("GreenSpawns"):GetChildren()

		local randomRedSpawn = RedSpawns[math.random(1,#RedSpawns)]
		local randomGreenSpawn = GreenSpawns[math.random(1,#GreenSpawns)]

		-- choose seeker(s).
		for i = seekerCount, 1, -1 do
			local random = math.random(1, #Participants)
			local playerPicked = Participants[random]

			if seekerCount == 1 then -- variety in small servers.
				if previousSeeker and playerPicked == previousSeeker then
					repeat 
						random = math.random(1, #Participants)
						playerPicked = Participants[random]
					until playerPicked ~= previousSeeker
				end

				previousSeeker = playerPicked
			end

			playerPicked.Team = Settings.Red
			
			local humanoid = playerPicked.Humanoid
			humanoid.HealthDisplayDistance = Settings.RunnerNameDistance
			humanoid.NameDisplayDistance = Settings.RunnerNameDistance
			
			local root = playerPicked.Character.HumanoidRootPart
			root.CFrame = randomRedSpawn.CFrame + Vector3.new(math.random(1,3),math.random(1,3),math.random(1,3))

			table.remove(Participants, random)
		end

		-- put rest in runners.
		for _, player: Player in pairs(Participants) do
			player.Team = Settings.Green

			local root = player.Character.HumanoidRootPart
			root.CFrame = randomGreenSpawn.CFrame + Vector3.new(math.random(1,3),math.random(1,3),math.random(1,3))

			local humanoid = player.Character.Humanoid :: Humanoid
			
			humanoid.Died:Connect(function()
				player.Team = Settings.Dead
			end)
		end

		local function win()
			-- sfx
			PlaySound(Background.Victory)
			PlaySound(Background.Applause)

			-- delay before winner gets teleported to spawn.
			task.wait(Background.Victory.TimeLength + 1)

			Values:SetAttribute("Began", false) 
			isStart = false

			for _, player in next, Players:GetPlayers() do
				player.Team = nil -- makes it so you need to press play to participate again.

				if player.Character then
					local humanoid = player.Character.Humanoid
					local root = player.Character.HumanoidRootPart
					root.CFrame = workspace.Lobby.SpawnLocation.CFrame * CFrame.new(0,10,0)

					humanoid.HealthDisplayDistance = 50
					humanoid.NameDisplayDistance = 50
				end
			end
		end

		-- tables stay the same
		local runners = Settings.Green:GetPlayers()
		local seekers = Settings.Red:GetPlayers()

		-- game start
		for seconds = Settings.TimeLength, 1, -1 do
			local time = timeFormat(seconds)
			Values:SetAttribute("Status", time)

			-- updated tables
			local greenCount = Settings.Green:GetPlayers()
			local redCount = Settings.Red:GetPlayers()

			if #greenCount == 0 then -- seekers win / all runners dead
				Values:SetAttribute("Status", Settings.SeekerWinStatus)
				for _, runner: Player in pairs(runners) do
					local runnerData = DataManager:GetData(runner)

					if not runnerData or not runnerData.Statistics then -- player left the game
						continue
					end

					runnerData.Statistics.Loses += 1
				end

				for _, seeker: Player in pairs(seekers) do
					local seekerData = DataManager:GetData(seeker)

					if not seekerData or not seekerData.Statistics then 
						continue
					end

					seekerData.Statistics.Wins += 1
					seekerData.Statistics.Coins += Settings.SeekerWinCoins
				end

				win()

				break
			end

			if seconds == 0 or #redCount == 0 then -- runners win / game ended
				Values:SetAttribute("Status", Settings.RunnerWinStatus)

				for _, runner: Player in pairs(greenCount) do
					local runnerData = DataManager:GetData(runner)

					if not runnerData or not runnerData.Statistics then -- player left/  +1 lose regardless
						continue
					end

					runnerData.Statistics.Wins += 1
					runnerData.Statistics.Coins += Settings.RunnerWinCoins
				end

				for _, seeker: Player in pairs(redCount) do
					local seekerData = DataManager:GetData(seeker)

					if not seekerData or not seekerData.Statistics then  -- player left / +1 lose regard
						continue
					end

					seekerData.Statistics.Loses += 1
				end

				win()

				if #redCount == 0 then -- seeker left, end game.
					break
				end
			end

			task.wait(1)
		end
	end
end

function onDeadPlayerRemoved(player: Player)
	--/ remove player from queue.
	if table.find(Participants, player) then
		table.remove(Participants, table.find(Participants, player))
	end

	--/ if participants less than minimum after leave, stop intermission.
	if #Participants < Settings.MinimumPlayers and not Values:GetAttribute("Began") then
		isBreak = true
		Values:SetAttribute("Status", Settings.MinimumPlayersStatus)
	end
end

function gamePresync()
	Settings.Dead.PlayerAdded:Connect(onDeadPlayerAdded)
	Settings.Dead.PlayerRemoved:Connect(onDeadPlayerRemoved)
end

function GameService.KnitStart()
	gamePresync()
end

return GameService
#5

I mean, if it works, it works. But you shouldn’t put table.find() within a table.remove().

An example on how to replace nested table functions (optional):

function onDeadPlayerRemoved(player: Player)
    for key, value in Participants do
        if value == player then
            table.remove(Participants, key)
            break
        end
    end
...

Your code looks good, and it isn’t cluttered at all, and you’ve used comments well. Props to you

1 Like
#6

For rounds i usually create OOP object that operates on states, like IsRunning or Time and ect.

#7

try breaking down the onDeadPlayerAdded function and its nested “win” function into separate smaller functions to add some readability. theres a bunch of for loops doing different things so those could be a good start point