How do I improve my Round Loop function?

Help and Feedback Code Review
, , ,
#1

quite a lengthy read so be warned

What does the code do and what are you not satisfied with?

  • The code I have written below is the scripts for my round loop. I made it so that it loops continously when the conditions for running the round are still met and when they are no longer met, the server gets notified by this and immediately stops the function. This is why I am using a coroutine for this. There is a part of the round loop function that can ignore the coroutine conditions as the player might win the round properly, causing them to be the only player in the round.

Script Problems:

  • I’m not satisfied with using global variables between server and module scripts
  • I’m not satisfied with having to use a coroutine which triggers a function that runs as a while loop, which are three different features used for just one ‘function’
  • The round ending system is quite confusing and might not be foolproof (although it is necessary to reward players properly if they have killed everyone in the game

What potential improvements have you considered?

  • I’m trying to transfer the variables between scripts using parameters, the main server script cannot detect the change in the module script unless if I return the function (obviously I don’t want to do that)
  • I’m willing to sacrifice not being able to end the round immediately when there is not enough players for a simpler way to end the round, already coded in the game which is meant to reward players. That means only using a while loop instead of having to add a coroutine.

How (specifically) do you want to improve the code?

  • Removing all the global variables and putting them as parameters of the while loop
  • Make the conditions for ending the round simpler and more manageable to edit
  • Making the code more efficient would be good

Main server script for review:

-- Variables for Services
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")
local Teams = game:GetService("Teams")

-- Variables
local round = require(script.Round)
local Weapon = require(script.Weapon)

local Status = workspace.Info.Status
local Events = ReplicatedStorage.Events
local PlayingTeam = Teams.Playing
local IdleTeam = Teams.Idle

-- Variables for RemoteEvents
local ChangeTeam = Events:FindFirstChild("ChangeTeam")
local ChangeVisibility = Events:FindFirstChild("ChangeVisibility")

-- Variables that will take on a different value later in the Round Loop module script (necessary to put them here)
local GameLoop = nil
_G.ClonedMap = nil
_G.RoundActivated = false 

-- Default Value for the game Status
Status.Value = "Waiting for 2 players to start the round"

--[[ Function below would be activated to change the state of the round with a coroutine, if neither condition met
 then it ignores the if statements. This is the main focus of this script which handles the round
]]
local function activateRound()
	if #PlayingTeam:GetPlayers() >= 2 and _G.RoundActivated == false then
		-- If there's more than two players and the round hasn't started yet (activate the round)
		GameLoop = coroutine.create(round.GameLoop)
		coroutine.resume(GameLoop)
	elseif #PlayingTeam:GetPlayers() < 2 and _G.RoundActivated == true then
		-- If there's less than two players remaining and the round is still running (stop the round)
		coroutine.close(GameLoop)
		
		_G.RoundActivated = false
		ChangeVisibility:FireAllClients("MapGui", false)
		ChangeVisibility:FireAllClients("PlayingStatus", true)
		
		if _G.ClonedMap then
			_G.ClonedMap:Destroy()
			
			for i, instance in _G.SkyboxAspects do
				instance:Destroy()
			end
		end
		
		Status.Value = "Round paused, server has inadequate players to start round"
		
		task.wait(3)
		
		Status.Value = "Waiting for 2 players to start the round"
	end
end

--[[ All the code below are mainly used to check every time a player is added or removed from PlayingTeam. 
This is important as the coroutine needs to only run when there are two or more players in the PlayingTeam
so when someone leaves, the coroutine immediately stops (coroutine.close), whereas if PlayingTeam
 gets 2+ players, it immediately starts (coroutine.resume)
]]

-- Changes a player's team and counter for PlayingTeam and checks if round should be started/stopped
local function changeTeam(player, newTeam)
	if newTeam == "Playing" then
		player.Team = PlayingTeam
	else
		player.Team = IdleTeam
	end

	activateRound()
end

-- Connecting from client (as team change occurs with the click of a button in the client)
ChangeTeam.OnServerEvent:Connect(changeTeam)

-- As player joins affect the amount of players in PlayingTeam, the script also checks if round should be started/stopped
Players.PlayerAdded:Connect(function()
	activateRound()
end)

Players.PlayerRemoving:Connect(function()
	activateRound()
end)

Round script (handles the loop that the main server script triggers and its contents)

-- Variables
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ServerStorage = game:GetService("ServerStorage")
local Players = game:GetService("Players")
local Lighting = game:GetService("Lighting")
local Teams = game:GetService("Teams")

local round = {}

local Weapon = require(script.Parent.Weapon)
local Events = ReplicatedStorage.Events
local Maps = ServerStorage:WaitForChild("Maps")

local ChangeVisibility = Events:FindFirstChild("ChangeVisibility")

local Status = workspace.Info.Status
local Playing = Teams.Playing

local gameReward = 100
local GameLength = 60
local intermission = 10
local continueRound = false

-- The function for the game loop, it keeps looping it gets activated
function round.GameLoop()
	while true do
		-- Set RoundActivated to true (cannot create another round) and do countdown to begin round
		_G.RoundActivated = true

	       -- deleted code that handles round intermission (not important for this post)

		local AvailableMaps = Maps:GetChildren()
		local SelectedMap = AvailableMaps[math.random(1, #AvailableMaps)]

		_G.ClonedMap = SelectedMap:Clone()
		_G.ClonedMap.Parent = workspace

		-- deleted code that handles PlayingTeam players' teleportation to the cloned map (not important)

		-- Preparing for the fight with a countdown
		for i = 3, 1, -1 do
			Status.Value = "Fight starts in " .. i
			task.wait(1)
		end
		
		-- coroutine cannot end without giving the last standing player a bonus anymore
		_G.RoundActivated = "nil"
		
		-- deleted code that handles the battle (not important)

-- Counting down the round length whilst checking the amount of players in the battle
        for seconds = GameLength, 0, -1 do
			-- Checking to see if the player is still in the round and removing them for the PlayersList table if they aren't
			for i, player in pairs(playersList) do	
				if player then
					local character = player.Character

					if character then
						if character:FindFirstChild("GameTag") then
							-- The player is alive
							print(player.Name .. " is still alive")
						else
							-- The player is dead
							table.remove(playersList, i)
							print(player.Name .. " has been removed")
						end
					else
						-- The player left the game
						table.remove(playersList, i)
						print(player.Name .. " has been removed")
					end
				else 
					-- If the player left the game
					table.remove(playersList, i)
					print(player.Name .. " has been removed")
				end
			end

			-- Counting down towards 0
			Status.Value = seconds .. " seconds remaining. " .. #playersList .. " players remain standing."
             if #playersList == 1 then
				-- Last person standing

				if #Players:GetPlayers() <= 1 then
					-- If the second player left the game (to avoid giving player 1 an unnecessary advantage)
					Status.Value = "Not enough players to continue to round"

					task.wait(2)

					Status.Value = playersList[1].Name .. " will be rewarded a fraction of the winning reward"
					playersList[1].leaderstats.Cash.Value += gameReward / 20
					break
				end
				
				-- If it was a fair victory
				Status.Value = playersList[1].Name .. " has won the round!"
				playersList[1].leaderstats.Cash.Value += gameReward
				break
			elseif #playersList == 0 then 
				-- No one survived the round
				Status.Value = "No one survived"
				break
			elseif seconds == 0 then
				-- Time runs out
				Status.Value = "Time's up!"
	
				task.wait(3)

				Status.Value = "Nobody won the round"
				for integer, survivor in pairs(playersList) do
					survivor.leaderstats.Cash.Value += gameReward / 5
				end

				break
			end

			task.wait(1)
		end

		Status.Value = "Clearing map..."

		-- the script removed any items the players use inside the round and teleporting them back to the lobby (deleted)
		
		-- Returning to normal lobby conditions and removing the map lighting
		_G.ClonedMap:Destroy()
		for i, instance in _G.SkyboxAspects do
			instance:Destroy()
		end
		
		-- If there are less than 2 players then the round will not repeat (without closing the coroutine)
		if #Players:GetPlayers() < 1 then
			Status.Value = "Waiting for 2 players to start the round"
			
			_G.RoundActivated = false
			break
		end
	end
end

return round
1 Like