Feedback on Round system

Help and Feedback Code Review
#1

Provide an overview of:

  • What does the code do and what are you not satisfied with?
    Hello! So currently, I have a script that does the intermission, round, and goes back to the lobby. This cycle repeats forever. In the module, the script doesn’t necessarily wait until say the round is over to do the cleanup.
  • What potential improvements have you considered?
    Having booleans that change values and then checking if those booleans were changed.
  • How (specifically) do you want to improve the code?
    I would like to make the code determine whether or not a round has actually ended instead of waiting at a fixed number like 16 seconds before proceeding to the cleanup and starting the next intermission.
local mod = {}

local minimumPlayers = 1

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local RemoteEvents = ReplicatedStorage:FindFirstChild("RemoteEvents")
local Skydive = RemoteEvents:WaitForChild("Skydive")


function mod:Intermission(Players, Status, Intermission, IntermissionLength)
	local function EnoughPlayers()
		for i = 0, 3 do
			Status.Value = "Waiting for more players to join"..string.rep(".", i)
			task.wait(1)
		end
	end

	repeat EnoughPlayers() task.wait() until #Players:GetPlayers() >= minimumPlayers

	repeat task.wait() until #Players:GetPlayers() >= minimumPlayers
	-- set the intermission value to true
	Intermission.Value = true

	for i = IntermissionLength, 0, -1 do
		Status.Value = "Intermission: "..tostring(i)
		task.wait(1)
	end

	Intermission.Value = false
end




function mod:StartRound(Players, Status, InProgress, Arena, TeleportTo)
	-- start the round
	InProgress.Value = true
	Status.Value = "Round in progress!"

	for i, player in pairs(Players:GetPlayers()) do
		local Character = player.Character or player.CharacterAdded:Wait()

		if Character then
			local HumanoidRootPart = Character:FindFirstChild("HumanoidRootPart")
			if HumanoidRootPart then
				player:SetAttribute("InRound", true)
				if player:GetAttribute("InRound") then
					HumanoidRootPart:PivotTo(TeleportTo.CFrame)
					Skydive:FireClient(player)
				end
			end
		end
	end

	task.wait(2)
	Arena.BrickColor = BrickColor.random()
	task.wait(2)
	Arena.CanCollide = false
end




function mod:EndRound(Players, Status, Arena, Target, LobbySpawn, InProgress)
	-- end the round
	local Connection
	local Winners = {}
	local debounce = false

	Target.Touched:Connect(function(Hit)
		local Player = Players:GetPlayerFromCharacter(Hit.Parent)
		
		if Player then
			if not table.find(Winners, Player) then -- if it's not the same player.
				table.insert(Winners, Player) -- then insert them into the table.
			end
			
			local Character = Player.Character or Player.CharacterAdded:Wait()
			if Character then
				local HumanoidRootPart = Character:FindFirstChild("HumanoidRootPart")
				if HumanoidRootPart then
					Player:SetAttribute("InRound", false)
					HumanoidRootPart:PivotTo(LobbySpawn.CFrame + Vector3.new(0, 10, 0))
					Arena.CanCollide = true
					for i, winner in pairs(Winners) do
						if table.find(Winners, Player) then -- checks if the player is a winner.
							Status.Value = "Winner(s): "..winner.Name
							Player.leaderstats["Glory Points"].Value += math.random(100, 200)
							Player.leaderstats["Glory Coins"].Value += math.random(25, 50)
						end
						task.wait(1)
					end
					
					table.clear(Winners) -- clear the table of previous winners.
					InProgress.Value = false
				end
			end
		end
	end)
end




function mod:Cleanup(Players, Status, Cleanup)
	-- cleanup and wait for the next round
	
	task.wait(16)

	Cleanup.Value = true

	for i = 0, 3 do
		Status.Value = "Cleaning up"..string.rep(".", i)
		task.wait(1)
	end

	Cleanup.Value = false
end




return mod
2 Likes
#2 
-- A round system is a sequence of events
-- it tells the program when certain parts of it should execute
-- that said, i believe there is no better way to do it than this way

local gameMode: string

-- using heap memory
while true do
    waitForPlayers();
    freeRoamTime();
    intermission();
    pickRandomGameMode();
    pickRandomMap();
    movePlayers();
    round();
end

-- using stack memory
while true do
    local players = waitForPlayers()
    local intermissionTime = intermission()
    local map = getRandomMap()
    movePlayers(players, map)
    round(players, map)
end

because order matters, pickRandomMap() would simply replace the map value so that movePlayers() can use the new value to find the right spawns for the right map

-- this is an example, it should be more robust
-- this method uses heap memory which is less optimized than stack memory

local gameMode: string
local map: string

local function pickRandomGameMode()
   gameMode = gamemodes[math.random(#gamemodes)]
end

local function pickRandomMap()
  local maps = gModeMapIndex[gamemode]
  map = maps[math.random(#maps )]
end

local function movePlayers()
  local spawns = map.spawns:GetChildren()
  for _, player in Players:GetPlayers() do
      if not player.Character then continue end
      player : AddTag 'playing'
      player.Character:PivotTo(spawns[math.random[#spawns]])
  end
end

this is bad, it abstracts away too much

might as well just code what’s in round in the main script, right?

local round = require(...)
round.start()

this is nice

i still prefer the first code in this reply with all the round related functions above the loop.

local round = require(...)

while true do
    round.waitForPlayers();
    round.freeRoamTime();
    round.intermission();
    round.etc();
end

also nice

-- tables -> {} -> {Player = {}}
local metrics = require(...)
local inventory= require(...)

local function giveReward()
   for player, kd in metrics do
        inventory[player].cash += kd.kills
   end
end

saving the inventory

-- extremely simple because it's an example
local inventory = require(...)
updateAsync(key, function()
     return inventory[player]
end)

Why did I shared the metrics, inventory bit? Because a lot of people would over complicate it too much with signals or BindableEvents, 100 classes, attributes, valuebase objects, connections just to share data when that is the job of a module.

for reusability of data and functions, you don’t have to build a class for each thing you do in your game, especially when that is only used once in your codebase.

you make a table accessible to other scripts then it absolutely slaps in many ways.

2 Likes
#3

Hello! Just want to point out the nested parts of code

So when you nest/indent code a lot, it becomes hard to keep track of how you reach that part of the code. Often when writing functions, you should try to make each function do only 1 thing, you don’t always have to do this, but it helps keep the code clean and more readable.

So what you can do to make it more readable is to use early return

Here is a function in your code (scroll down for the way i would do it)

function mod:EndRound(Players, Status, Arena, Target, LobbySpawn, InProgress)
	-- end the round
	local Connection
	local Winners = {}
	local debounce = false

	Target.Touched:Connect(function(Hit)
		local Player = Players:GetPlayerFromCharacter(Hit.Parent)

		if Player then
		    if not table.find(Winners, Player) then -- if it's not the same player.
			    table.insert(Winners, Player) -- then insert them into the table.
		    end
		    
		    local Character = Player.Character or Player.CharacterAdded:Wait()
		    if Character then
			    local HumanoidRootPart = Character:FindFirstChild("HumanoidRootPart")
			    if HumanoidRootPart then
				    Player:SetAttribute("InRound", false)
				    HumanoidRootPart:PivotTo(LobbySpawn.CFrame + Vector3.new(0, 10, 0))
				    Arena.CanCollide = true
				    for i, winner in pairs(Winners) do
					    if table.find(Winners, Player) then -- checks if the player is a winner.
						    Status.Value = "Winner(s): "..winner.Name
						    Player.leaderstats["Glory Points"].Value += math.random(100, 200)
						    Player.leaderstats["Glory Coins"].Value += math.random(25, 50)
					    end
					    task.wait(1)
				    end
				    
				    table.clear(Winners) -- clear the table of previous winners.
				    InProgress.Value = false
			    end
		    end
		end
	end)
end

Using return early principle
Explanation, whenever there is an if statement that contains the rest of the code in the function. You can just reverse the if statement.

Example

if variable == 100 then
	--Some code
end

You can do

if not variable == 100 then 
	return
end
--put code inside the if statement afterwards

Notice how this is much more readable and easier to understand.
If you find that your not able to do an early return, you should consider creating a new function.
I usually only have 1-4 indents max. And if it becomes to indented which rarly occours I just create a new function and put the indented code in there

function mod:EndRound(Players, Status, Arena, Target, LobbySpawn, InProgress)
	-- end the round
	local Connection
	local Winners = {}
	local debounce = false

	Target.Touched:Connect(function(Hit)
		local Player = Players:GetPlayerFromCharacter(Hit.Parent)
		local Character = Player.Character or Player.CharacterAdded:Wait()
		local HumanoidRootPart = Character:FindFirstChild("HumanoidRootPart")

		if not Player then
			return
		end
		if not table.find(Winners, Player) then -- if it's not the same player.
			table.insert(Winners, Player) -- then insert them into the table.
		end
		if not Character then
            return
		end
		if not HumanoidRootPart then
            return
		end

		Player:SetAttribute("InRound", false)
		HumanoidRootPart:PivotTo(LobbySpawn.CFrame + Vector3.new(0, 10, 0))
		Arena.CanCollide = true
		for i, winner in pairs(Winners) do
			if table.find(Winners, Player) then -- checks if the player is a winner.
				Status.Value = "Winner(s): "..winner.Name
				Player.leaderstats["Glory Points"].Value += math.random(100, 200)
				Player.leaderstats["Glory Coins"].Value += math.random(25, 50)
			end
			task.wait(1)
		end
		
		table.clear(Winners) -- clear the table of previous winners.
		InProgress.Value = false
	end)
end

Best of luck and have fun coding! :slight_smile:

2 Likes