Round-type module looks messy imp

[Remingling]

Hi I am working on a round game with different gamemodes and each gamemode runs on a module script.

I am working on classic gamemode where its basically runners vs killer.

After finishing my script I looked back at it and saw how hard and stressful it would be to add new things to the script. Is there anyway to optimize this script furthermore so anytime I edit it I don’t get a headache?

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

local Remotes = ReplicatedStorage.Remotes.Classic
local Values = ReplicatedStorage.Values
local Voting = workspace.Lobby.Voting

local Maps = ServerStorage.Maps:GetChildren()
local Choices = Voting:GetChildren()

local TimeFormat = require(ReplicatedStorage.Modules.TimeFormat)

local Settings = {
	Round = {
		Interval = 15,
		RoundTime = 125
	};
	
	Strings = {
		NotEnoughPlayers = "Waiting for more players...",
		Starting = "Starting in: ",
		Ending = false,
	}
}

local Teams = {
	Red = game.Teams.Killer,
	Blue = game.Teams.Runner,
	Lobby = game.Teams.Lobby
}

local Status = Values.Status
local InRound = Values.InRound

-- declarations

local isAnOption
local randomMap
local chosenMap
local mapClone
local previousKiller
local killer

local BlueCount
local RedCount
local RemainderBlue
local RemainderRed

local Connections = {}
local Playerlist = {}
local Jukebox = {}
Jukebox.__index = Jukebox

-- functions

function CheckIfMenu(Player)
	if Player.PlayerGui.Menu.Frame.Visible == true then
		return false
	else
		return true
	end
end


function PickRandomMap() -- Self Explanatory
	local randomNumber = math.random(1, #Maps)
	randomMap = Maps[randomNumber]
	return randomMap.CanBeVoted
end

function PickKiller()
	for _, player in ipairs(game.Players:GetChildren()) do  
		if player ~= previousKiller then
			if player.PlayerGui.Camera.Enabled == true then
				table.insert(Playerlist, player)
			end
		end
	end
	
	print(Playerlist)
	local SelectedKiller = Playerlist[math.random(1, #Playerlist)]
	previousKiller = SelectedKiller
	Playerlist = {}
	return SelectedKiller
end

function VoteSystem()
	for i, choice in pairs(Choices) do
		local name = choice.label.SurfaceGui.TextLabel
		local picture = choice.Image.SurfaceGui.ImageLabel
		isAnOption = PickRandomMap()
		if isAnOption.Value == true then
			repeat 
				isAnOption = PickRandomMap()
			until isAnOption.Value == false
			name.Text = randomMap.Name
			picture.Image = randomMap.Image.Value
			randomMap.CanBeVoted.Value = true
		else
			name.Text = randomMap.Name
			picture.Image = randomMap.Image.Value
			randomMap.CanBeVoted.Value = true		
		end					
	end	
end

function RoundChanged()
	if InRound.Value == true then -- round is in progress
		local Choice1Votes = #Voting.Choice1.button.Votes:GetChildren()
		local Choice2Votes = #Voting.Choice2.button.Votes:GetChildren()
		local Choice3Votes = #Voting.Choice3.button.Votes:GetChildren()
		
		if Choice1Votes >= Choice2Votes and Choice1Votes >= Choice3Votes then
			chosenMap = Voting.Choice1.label.SurfaceGui.TextLabel.Text
		elseif Choice2Votes >= Choice1Votes and Choice2Votes >= Choice3Votes then
			chosenMap = Voting.Choice2.label.SurfaceGui.TextLabel.Text
		else
			chosenMap = Voting.Choice3.label.SurfaceGui.TextLabel.Text
		end
		
		Status.Value = "Map: ".. chosenMap
		
		for _, map in pairs(Maps) do
			if chosenMap == map.Name then
				mapClone = map:Clone()
				mapClone.Parent = game.Workspace
			end
		end	
		
		for i, choice in pairs(Choices) do
			choice.label.SurfaceGui.TextLabel.Text = " "
			choice.Image.SurfaceGui.ImageLabel.Image = " "
			choice.button.Votes:ClearAllChildren()
			Remotes.VoteReset:FireAllClients(choice.button)
		end
		
		local BlueTeamCount = {}
		local RedTeamCount = {}
		local killer = PickKiller()
		
		if killer then
			local RedSpawns = mapClone.RedSpawns:GetChildren()
			local BlueSpawns = mapClone.BlueSpawns:GetChildren()
			for i, plr in ipairs(game.Players:GetChildren()) do
				local char = plr.Character
				local humanRoot = char:WaitForChild("HumanoidRootPart")
				-- picks a random spawn from each team
				local randomRedSpawn = RedSpawns[math.random(1,#RedSpawns)]
				local randomBlueSpawn = BlueSpawns[math.random(1,#BlueSpawns)]
				-- will put the current player into a team with the less amount of players
				if plr ~= killer then
					plr.Team = Teams.Blue
					humanRoot.CFrame = randomBlueSpawn.CFrame + Vector3.new(math.random(1,3),math.random(1,3),math.random(1,3))
					plr.CameraMode = Enum.CameraMode.Classic
					plr.CameraMaxZoomDistance = 9
					table.insert(BlueTeamCount, plr.Name)
				elseif plr == killer then
					plr.Team = Teams.Red
					plr.CameraMode = Enum.CameraMode.LockFirstPerson
					plr.CameraMaxZoomDistance = 9
					humanRoot.CFrame = randomRedSpawn.CFrame + Vector3.new(math.random(1,3),math.random(1,3),math.random(1,3))
					table.insert(RedTeamCount, plr.Name)
				end
			end
		end
	else -- not in progress
		for _, player in pairs(Players:GetPlayers()) do
			local Character = player.Character or player.CharacterAdded:Wait()
			if Character then -- just incase
				local HumanoidRootPart = Character:FindFirstChild("HumanoidRootPart")
				HumanoidRootPart.CFrame = (workspace.Lobby.lobbySpawn.CFrame + Vector3.new(0,5,0))
				player.Team = Teams.Lobby
				if Character.Head:FindFirstChild("Killbrick") then
					Character.Head:FindFirstChild("Killbrick"):Destroy() -- destroys the kill brick
				end
			end
		end
		-- voting system
		if mapClone ~= nil then
			mapClone:Destroy()
		end
		
		for i, map in pairs(Maps) do
			map.CanBeVoted.Value = false
		end

		for i, choice in pairs(Choices) do
			local name = choice.label.SurfaceGui.TextLabel
			local picture = choice.Image.SurfaceGui.ImageLabel
			isAnOption = PickRandomMap()
			if isAnOption.Value == true then
				repeat 
					isAnOption = PickRandomMap()
				until isAnOption.Value == false
				name.Text = randomMap.Name
				picture.Image = randomMap.Image.Value
				randomMap.CanBeVoted.Value = true
			else
				name.Text = randomMap.Name
				picture.Image = randomMap.Image.Value
				randomMap.CanBeVoted.Value = true		
			end	
		end
	end
end
-----
function Jukebox.new()
	local self = setmetatable({},Jukebox)
	
	return self
end

function Jukebox:Round()
	while true do
		
		repeat 
			Status.Value = Settings.Strings.NotEnoughPlayers
			task.wait(1)
		until #Players:GetChildren() >= 1
		
		InRound.Value = false
		
		for i = Settings.Round.Interval,0,-1 do
			Status.Value = Settings.Strings.Starting..i
			task.wait(1)
		end
		InRound.Value = true
		
		BlueCount = {}
		RedCount = {}
		
		for i = Settings.Round.RoundTime,0,-1 do
			task.wait(1)
			local Time = TimeFormat:Convert(i, "Default", true)
			if Settings.Strings.Ending == false then
				Status.Value = Time
			else
				Status.Value = Settings.Strings.Ending..Time
			end
			

			local RemainderBlue = {}
			local RemainderRed = {}

			for _, player in pairs(Players:GetPlayers()) do
				if player.Team == Teams.Blue then
					if i == Settings.Round.RoundTime then
						table.insert(BlueCount,player.Name)
					end
					table.insert(RemainderBlue,player.Name)
				elseif player.Team == Teams.Red then
					if i == Settings.Round.RoundTime then
						table.insert(RedCount,player.Name)
					end
					table.insert(RemainderRed, player.Name)
				end	
			end
			
			if #RemainderRed == 0 and i == 0 then -- both teams lost
				print("Both lost")
				break
			end
			
			if #RemainderRed == 0 or i == 0 then -- blue won
				print("Blue won")
				break
			end
			
			if #RemainderBlue == 0 then -- red won
				print("Red won")
				break
			end
		end
	end
end

-- connections
Values.InRound:GetPropertyChangedSignal("Value"):Connect(RoundChanged)

game.Players.PlayerAdded:Connect(function(Player)
	script.Loading:Clone().Parent = Player.PlayerGui -- loads game
	Player:GetPropertyChangedSignal("Team"):Connect(function()
		local Killbrick = ServerStorage.Assets.Red.KillBrick
		if Player and Player.Team == Teams.Red then -- if players red
			local character = Player.Character
			if character then
				Player.CameraMode = "LockFirstPerson"
				local kb = Killbrick:Clone()
				wait(0)
				kb.Parent = character.Head
				kb.CFrame = (character.Head.CFrame + Vector3.new(0,0,-2))
				kb.Orientation = Vector3.new (90,0,0)
				local weld = Instance.new("WeldConstraint")
				weld.Parent = character.Head
				weld.Part0 = kb
				weld.Part1 = character.Head

				kb.IsInUse.Value = true
			end

			Connections[Player] = Player.CharacterAdded:Connect(function(character)
				local kb = Killbrick:Clone()

				task.wait(0)
				kb.Parent = character.Head
				kb.CFrame = (character.Head.CFrame + Vector3.new(0,0,2))
				kb.Orientation = Vector3.new (90,0,0)
				local weld = Instance.new("WeldConstraint")
				kb.Name = "Killbrick"
				weld.Name = "Weld"
				weld.Parent = character.Head
				weld.Part0 = kb
				weld.Part1 = character.Head
				Player.CameraMode = "LockFirstPerson"

				kb.IsInUse.Value = true

			end)
		else
			local connection = Connections[Player]

			if connection then
				connection:Disconnect()
			end

			local character = Player.Character

			if character then
				for i,v in pairs(character.Head:GetChildren()) do
					if v.Name == ("Killbrick") then
						v:Destroy()
					end
				end
			end
		end
	end)
	
	Player.CharacterAdded:Connect(function(Character)
		Character.Humanoid.Died:Connect(function()
			Player.Team = Teams.Lobby
			Player.CameraMode = Enum.CameraMode.Classic
			Player.CameraMaxZoomDistance = 9
		end)
	end)
end)

-- initilizing gamemode
VoteSystem()
Jukebox:Round()

return Jukebox

Thanks for reading.

[ThanksRoBama (Emma)]

There’s a lot of things that can be cleaned up so your basic code style gets improved. Try installing selene which is a Roblox Luau linter. It highlights problems in your code. For example:

The ones underlined in red are those “unused variable” warnings. They let you know that you forgot to delete a variable or maybe you have a bug because of defining a local variable that shadows a variable from an outer scope.

Selene gives 16 warnings for your script.

Here’s another example:

i is unused so it should be an underscore instead.

These two were defined at the top level, here you’re redefining them with local meaning you’re shadowing the variables from the top level. Plus they’re unused:

You are inserting values, but never reading anything from the table so it’s useless.

Here selene complains that you’re shadowing a variable (this time actually in use):

It’s not a huge deal, but a better name would be addedCharacter to remove this ambiguity.

Fixing these problems doesn’t make things more readable, but it’s the first step to be done before refactoring.

Here’s a version with those fixes implemented, I’ll use it for part two in the next comment:

Code

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

local Remotes = ReplicatedStorage.Remotes.Classic
local Values = ReplicatedStorage.Values
local Voting = game.Workspace.Lobby.Voting

local Maps = ServerStorage.Maps:GetChildren()
local Choices = Voting:GetChildren()

local TimeFormat = require(ReplicatedStorage.Modules.TimeFormat)

local Settings = {
	Round = {
		Interval = 15,
		RoundTime = 125
	},
	Strings = {
		NotEnoughPlayers = "Waiting for more players...",
		Starting = "Starting in: ",
		Ending = false,
	}
}

local Teams = {
	Red = game.Teams.Killer,
	Blue = game.Teams.Runner,
	Lobby = game.Teams.Lobby
}

local Status = Values.Status
local InRound = Values.InRound

-- Declarations

local isAnOption
local randomMap
local chosenMap
local mapClone
local previousKiller

local BlueCount
local RedCount

local Connections = {}
local Playerlist = {}
local Jukebox = {}
Jukebox.__index = Jukebox

-- Functions
function PickRandomMap() -- Self Explanatory
	local randomNumber = math.random(1, #Maps)
	randomMap = Maps[randomNumber]
	return randomMap.CanBeVoted
end

function PickKiller()
	for _, player in ipairs(game.Players:GetPlayers()) do  
		if player ~= previousKiller then
			if player.PlayerGui.Camera.Enabled == true then
				table.insert(Playerlist, player)
			end
		end
	end
	
	local SelectedKiller = Playerlist[math.random(1, #Playerlist)]
	previousKiller = SelectedKiller
	Playerlist = {}
	return SelectedKiller
end

function VoteSystem()
	for _, choice in pairs(Choices) do
		local name = choice.label.SurfaceGui.TextLabel
		local picture = choice.Image.SurfaceGui.ImageLabel
		isAnOption = PickRandomMap()
		if isAnOption.Value == true then
			repeat 
				isAnOption = PickRandomMap()
			until isAnOption.Value == false
			name.Text = randomMap.Name
			picture.Image = randomMap.Image.Value
			randomMap.CanBeVoted.Value = true
		else
			name.Text = randomMap.Name
			picture.Image = randomMap.Image.Value
			randomMap.CanBeVoted.Value = true		
		end					
	end	
end

function RoundChanged()
	if InRound.Value == true then -- round is in progress
		local Choice1Votes = #Voting.Choice1.button.Votes:GetChildren()
		local Choice2Votes = #Voting.Choice2.button.Votes:GetChildren()
		local Choice3Votes = #Voting.Choice3.button.Votes:GetChildren()
		
		if Choice1Votes >= Choice2Votes and Choice1Votes >= Choice3Votes then
			chosenMap = Voting.Choice1.label.SurfaceGui.TextLabel.Text
		elseif Choice2Votes >= Choice1Votes and Choice2Votes >= Choice3Votes then
			chosenMap = Voting.Choice2.label.SurfaceGui.TextLabel.Text
		else
			chosenMap = Voting.Choice3.label.SurfaceGui.TextLabel.Text
		end
		
		Status.Value = "Map: ".. chosenMap
		
		for _, map in pairs(Maps) do
			if chosenMap == map.Name then
				mapClone = map:Clone()
				mapClone.Parent = game.Workspace
			end
		end	
		
		for _, choice in pairs(Choices) do
			choice.label.SurfaceGui.TextLabel.Text = " "
			choice.Image.SurfaceGui.ImageLabel.Image = " "
			choice.button.Votes:ClearAllChildren()
			Remotes.VoteReset:FireAllClients(choice.button)
		end
		
		local killer = PickKiller()
		
		if killer then
			local RedSpawns = mapClone.RedSpawns:GetChildren()
			local BlueSpawns = mapClone.BlueSpawns:GetChildren()
			for _, plr in ipairs(game.Players:GetChildren()) do
				local char = plr.Character
				local humanRoot = char:WaitForChild("HumanoidRootPart")
				-- picks a random spawn from each team
				local randomRedSpawn = RedSpawns[math.random(1,#RedSpawns)]
				local randomBlueSpawn = BlueSpawns[math.random(1,#BlueSpawns)]
				-- will put the current player into a team with the less amount of players
				if plr ~= killer then
					plr.Team = Teams.Blue
					humanRoot.CFrame = randomBlueSpawn.CFrame + Vector3.new(math.random(1,3),math.random(1,3),math.random(1,3))
					plr.CameraMode = Enum.CameraMode.Classic
					plr.CameraMaxZoomDistance = 9
				elseif plr == killer then
					plr.Team = Teams.Red
					plr.CameraMode = Enum.CameraMode.LockFirstPerson
					plr.CameraMaxZoomDistance = 9
					humanRoot.CFrame = randomRedSpawn.CFrame + Vector3.new(math.random(1,3),math.random(1,3),math.random(1,3))
				end
			end
		end
	else -- not in progress
		for _, player in pairs(Players:GetPlayers()) do
			local Character = player.Character or player.CharacterAdded:Wait()
			if Character then -- just incase
				local HumanoidRootPart = Character:FindFirstChild("HumanoidRootPart")
				HumanoidRootPart.CFrame = (workspace.Lobby.lobbySpawn.CFrame + Vector3.new(0,5,0))
				player.Team = Teams.Lobby
				if Character.Head:FindFirstChild("Killbrick") then
					Character.Head:FindFirstChild("Killbrick"):Destroy() -- destroys the kill brick
				end
			end
		end
		-- voting system
		if mapClone ~= nil then
			mapClone:Destroy()
		end
		
		for _, map in pairs(Maps) do
			map.CanBeVoted.Value = false
		end

		for _, choice in pairs(Choices) do
			local name = choice.label.SurfaceGui.TextLabel
			local picture = choice.Image.SurfaceGui.ImageLabel
			isAnOption = PickRandomMap()
			if isAnOption.Value == true then
				repeat 
					isAnOption = PickRandomMap()
				until isAnOption.Value == false
				name.Text = randomMap.Name
				picture.Image = randomMap.Image.Value
				randomMap.CanBeVoted.Value = true
			else
				name.Text = randomMap.Name
				picture.Image = randomMap.Image.Value
				randomMap.CanBeVoted.Value = true		
			end	
		end
	end
end
-----
function Jukebox.new()
	local self = setmetatable({},Jukebox)
	
	return self
end

function Jukebox:Round()
	while true do
		
		repeat 
			Status.Value = Settings.Strings.NotEnoughPlayers
			task.wait(1)
		until #Players:GetChildren() >= 1
		
		InRound.Value = false
		
		for i = Settings.Round.Interval,0,-1 do
			Status.Value = Settings.Strings.Starting..i
			task.wait(1)
		end
		InRound.Value = true
		
		BlueCount = {}
		RedCount = {}
		
		for i = Settings.Round.RoundTime,0,-1 do
			task.wait(1)
			local Time = TimeFormat:Convert(i, "Default", true)
			if Settings.Strings.Ending == false then
				Status.Value = Time
			else
				Status.Value = Settings.Strings.Ending..Time
			end
			

			local RemainderBlue = {}
			local RemainderRed = {}

			for _, player in pairs(Players:GetPlayers()) do
				if player.Team == Teams.Blue then
					if i == Settings.Round.RoundTime then
						table.insert(BlueCount,player.Name)
					end
					table.insert(RemainderBlue,player.Name)
				elseif player.Team == Teams.Red then
					if i == Settings.Round.RoundTime then
						table.insert(RedCount,player.Name)
					end
					table.insert(RemainderRed, player.Name)
				end	
			end
			
			if #RemainderRed == 0 and i == 0 then -- both teams lost
				print("Both lost")
				break
			end
			
			if #RemainderRed == 0 or i == 0 then -- blue won
				print("Blue won")
				break
			end
			
			if #RemainderBlue == 0 then -- red won
				print("Red won")
				break
			end
		end
	end
end

-- Connections
Values.InRound:GetPropertyChangedSignal("Value"):Connect(RoundChanged)

game.Players.PlayerAdded:Connect(function(Player)
	script.Loading:Clone().Parent = Player.PlayerGui -- loads game
	Player:GetPropertyChangedSignal("Team"):Connect(function()
		local Killbrick = ServerStorage.Assets.Red.KillBrick
		if Player and Player.Team == Teams.Red then -- if players red
			local character = Player.Character
			if character then
				Player.CameraMode = "LockFirstPerson"
				local kb = Killbrick:Clone()
				wait(0)
				kb.Parent = character.Head
				kb.CFrame = (character.Head.CFrame + Vector3.new(0,0,-2))
				kb.Orientation = Vector3.new (90,0,0)
				local weld = Instance.new("WeldConstraint")
				weld.Parent = character.Head
				weld.Part0 = kb
				weld.Part1 = character.Head

				kb.IsInUse.Value = true
			end

			Connections[Player] = Player.CharacterAdded:Connect(function(addedCharacter)
				local kb = Killbrick:Clone()

				task.wait(0)
				kb.Parent = addedCharacter.Head
				kb.CFrame = (addedCharacter.Head.CFrame + Vector3.new(0,0,2))
				kb.Orientation = Vector3.new (90,0,0)
				local weld = Instance.new("WeldConstraint")
				kb.Name = "Killbrick"
				weld.Name = "Weld"
				weld.Parent = addedCharacter.Head
				weld.Part0 = kb
				weld.Part1 = addedCharacter.Head
				Player.CameraMode = "LockFirstPerson"

				kb.IsInUse.Value = true

			end)
		else
			local connection = Connections[Player]

			if connection then
				connection:Disconnect()
			end

			local character = Player.Character

			if character then
				for _,v in pairs(character.Head:GetChildren()) do
					if v.Name == ("Killbrick") then
						v:Destroy()
					end
				end
			end
		end
	end)

	Player.CharacterAdded:Connect(function(Character)
		Character.Humanoid.Died:Connect(function()
			Player.Team = Teams.Lobby
			Player.CameraMode = Enum.CameraMode.Classic
			Player.CameraMaxZoomDistance = 9
		end)
	end)
end)

-- Initilize
VoteSystem()
Jukebox:Round()

return Jukebox

[ThanksRoBama (Emma)]

I see a lot of code duplication, so the next step is getting rid of that where it’s appropriate (it’s not always appropriate, but often). This is to make your code DRYer (don’t repeat yourself), which is usually better. I’ll also fix minor mistakes along the way.

You have some places where you pick a random item from a collection of items. That’s used all over the place, so let’s write a reusable function instead of writing out the logic every time:

function PickRandom<T>(items: {T}): T
	return items[math.random(1, #items)]	
end

A minor mistake is to use pairs where ipairs would work. It’s slightly slower, but the important thing is that using ipairs tells the reader something important about the collection, that it’s an array and not a dictionary. So use ipairs where you can. E.g.

These 3 lines are repeated for no reason:

I rarely use == to check values if I know they’re booleans and I just want to check truthy-ness:

The isAnOption loop stuff can be simplified to

function VoteSystem()
	for _, choice in ipairs(Choices) do
		local name = choice.label.SurfaceGui.TextLabel
		local picture = choice.Image.SurfaceGui.ImageLabel
		
		repeat
			isAnOption = PickRandomMap()
		until not isAnOption.Value			

		name.Text = randomMap.Name
		picture.Image = randomMap.Image.Value
		randomMap.CanBeVoted.Value = true
	end	
end

---

chosenMap is a bad name for this variable, because it’s not actually a map but the name of a map. In the highlighted situation that causes confusion because the variable suggests that’s it’s a map and not a string. chosenMapName is a better name. You should also break out of the loop when you find the item of interest. It’s slightly faster, and once again improves readability by a lot. It’s also better IMO to use an early continue instead of an if statement, to reduce nesting:

Again, DRY (especially important for logic like the CFrame stuff):

No, figure out how the engine works and then write code confidently:

Idiomatic one-liner for destroying a thing if it exists:

This is the exact logic that VoteSystem describes, so just call that instead:

You’re repeating the logic for setting up a killbrick, so put it in a function and call that:

function GiveKillBrick(player: Player): Part
	local character = player.Character
	assert(character, `Cannot give kill brick to player without a character ({player.DisplayName})!`)

	local killBrick = ServerStorage.Assets.Red.KillBrick:Clone()
	killBrick:PivotTo(character.Head:GetPivot() + Vector3.new(0, 0, -2))
	killBrick.Orientation = Vector3.new(90, 0, 0) --TODO: Just modify the pivot of the killBrick so you don't need this. Or just swap the size's axes.
	
	killBrick.Weld.Part1 = character.Head --NOTE: No need to set it up in code, just have the weld as part of the prefab.
	killBrick.Parent = character.Head

	return killBrick
end

This leaves some additional repetition:

In all my projects I end up with a cleanup function like this (with some more logic for tables but w/e):

function Clean(item: nil|Instance|RBXScriptConnection)
	if typeof(item) == nil then
		return
	elseif typeof(item) == "RBXScriptConnection" then
		if not item.Connected then return end
		item:Disconnect()
	elseif typeof(item) == "Instance" then
		Debris:AddItem(item, 0)
	else
		error(`Cannot clean item of unknown type '{typeof(item)}'`)
	end
end

Here’s a version with these fixes made and some minor ones, plus some comments explaining the fixes:

Summary

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

local Remotes = ReplicatedStorage.Remotes.Classic
local Values = ReplicatedStorage.Values
local Voting = game.Workspace.Lobby.Voting

local Maps = ServerStorage.Maps:GetChildren()
local Choices = Voting:GetChildren()

local TimeFormat = require(ReplicatedStorage.Modules.TimeFormat)

local Settings = {
	Round = {
		Interval = 15,
		RoundTime = 125
	},
	Strings = {
		NotEnoughPlayers = "Waiting for more players...",
		Starting = "Starting in: ",
		Ending = false,
	}
}

local Teams = {
	Red = game.Teams.Killer,
	Blue = game.Teams.Runner,
	Lobby = game.Teams.Lobby
}

local Status = Values.Status
local InRound = Values.InRound

-- Declarations
local isAnOption
local randomMap
local chosenMapName
local mapClone
local previousKiller

local BlueCount
local RedCount

local Connections = {}
local Playerlist = {}
local Jukebox = {}
Jukebox.__index = Jukebox

-- Functions
function PickRandom<T>(items: {T}): T
	return items[math.random(1, #items)]	
end

function PickRandomMap() -- Self Explanatory
	randomMap = PickRandom(Maps)
	return randomMap.CanBeVoted
end

function PickKiller()
	for _, player in ipairs(game.Players:GetPlayers()) do  
		if player ~= previousKiller then
			if player.PlayerGui.Camera.Enabled == true then
				table.insert(Playerlist, player)
			end
		end
	end
	
	local killer = PickRandom(Playerlist)
	assert(killer, "Cannot choose a killer (not enough players who aren't the previous killler)!")
	return killer
end

function Clean(item: nil|Instance|RBXScriptConnection)
	if typeof(item) == nil then
		return
	elseif typeof(item) == "RBXScriptConnection" then
		if not item.Connected then return end
		item:Disconnect()
	elseif typeof(item) == "Instance" then
		Debris:AddItem(item, 0)
	else
		error(`Cannot clean item of unknown type '{typeof(item)}'`)
	end
end

function GiveKillBrick(player: Player): Part
	local character = player.Character
	assert(character, `Cannot give kill brick to player without a character ({player.DisplayName})!`)

	local killBrick = ServerStorage.Assets.Red.KillBrick:Clone()
	killBrick:PivotTo(character.Head:GetPivot() + Vector3.new(0, 0, -2))
	killBrick.Orientation = Vector3.new(90, 0, 0) --TODO: Just modify the pivot of the killBrick so you don't need this. Or just swap the size's axes.
	
	killBrick.Weld.Part1 = character.Head --NOTE: No need to set it up in code, just have the weld as part of the prefab.
	killBrick.Parent = character.Head

	return killBrick
end

function SetupPlayerForRedTeam(player: Player)
	local killBrick = GiveKillBrick(player)
	killBrick.IsInUse.Value = true --TODO: Is this actually used? Otherwise remove

	player.CameraMode = "LockFirstPerson"
end

function VoteSystem()
	for _, choice in ipairs(Choices) do
		local name = choice.label.SurfaceGui.TextLabel
		local picture = choice.Image.SurfaceGui.ImageLabel
		
		repeat
			isAnOption = PickRandomMap()
		until not isAnOption.Value			

		name.Text = randomMap.Name
		picture.Image = randomMap.Image.Value
		randomMap.CanBeVoted.Value = true
	end
end

function RoundChanged()
	if InRound.Value == true then -- round is in progress
		local Choice1Votes = #Voting.Choice1.button.Votes:GetChildren()
		local Choice2Votes = #Voting.Choice2.button.Votes:GetChildren()
		local Choice3Votes = #Voting.Choice3.button.Votes:GetChildren()
		
		if Choice1Votes >= Choice2Votes and Choice1Votes >= Choice3Votes then
			chosenMapName = Voting.Choice1.label.SurfaceGui.TextLabel.Text
		elseif Choice2Votes >= Choice1Votes and Choice2Votes >= Choice3Votes then
			chosenMapName = Voting.Choice2.label.SurfaceGui.TextLabel.Text
		else
			chosenMapName = Voting.Choice3.label.SurfaceGui.TextLabel.Text
		end
		
		Status.Value = "Map: " .. chosenMapName
		
		for _, map in ipairs(Maps) do
			if chosenMapName ~= map.Name then continue end
				
			mapClone = map:Clone()
			mapClone.Parent = game.Workspace
			break
		end	
		
		for _, choice in ipairs(Choices) do
			choice.label.SurfaceGui.TextLabel.Text = " "
			choice.Image.SurfaceGui.ImageLabel.Image = " "
			choice.button.Votes:ClearAllChildren()
			Remotes.VoteReset:FireAllClients(choice.button)
		end
		
		local killer = PickKiller()
		--NOTE: No if killer then check needed, since PickKiller returns a player or errors
		
		previousKiller = killer

		local RedSpawns = mapClone.RedSpawns:GetChildren()
		local BlueSpawns = mapClone.BlueSpawns:GetChildren()

		for _, player in ipairs(game.Players:GetChildren()) do
			local character = player.character
			local rootPart = character:WaitForChild("HumanoidRootPart")

			-- Pick a random spawn from each team
			local playerSpawn
			local randomRedSpawn = PickRandom(RedSpawns)
			local randomBlueSpawn = PickRandom(BlueSpawns)

			--Misc. setup for each player
			player.CameraMaxZoomDistance = 9

			-- Per-team setup for each player
			if player == killer then
				player.Team = Teams.Red
				player.CameraMode = Enum.CameraMode.LockFirstPerson
				playerSpawn = randomRedSpawn
			else
				player.Team = Teams.Blue
				player.CameraMode = Enum.CameraMode.Classic
				playerSpawn = randomBlueSpawn
			end
			rootPart.CFrame = playerSpawn.CFrame + Vector3.new(math.random(1, 3), math.random(1, 3), math.random(1, 3))
		end
	else -- not in progress
		for _, player in ipairs(Players:GetPlayers()) do
			local character = player.character or player.CharacterAdded:Wait()
			local rootPart = character:FindFirstChild("HumanoidRootPart")
			rootPart.CFrame = workspace.Lobby.lobbySpawn.CFrame + Vector3.new(0, 5, 0)
			player.Team = Teams.Lobby

			Clean(character.Head:FindFirstChild("Killbrick"))
		end

		-- voting system
		Clean(mapClone)

		for _, map in ipairs(Maps) do
			map.CanBeVoted.Value = false
		end

		VoteSystem()
	end
end

-----

function Jukebox.new()
	local self = setmetatable({}, Jukebox)
	return self
end

function Jukebox:Round()
	while true do
		
		repeat 
			Status.Value = Settings.Strings.NotEnoughPlayers
			task.wait(1)
		until #Players:GetChildren() >= 1
		
		InRound.Value = false
		
		for i = Settings.Round.Interval,0,-1 do
			Status.Value = Settings.Strings.Starting..i
			task.wait(1)
		end
		InRound.Value = true
		
		BlueCount = {}
		RedCount = {}
		
		for i = Settings.Round.RoundTime,0,-1 do
			task.wait(1)
			local Time = TimeFormat:Convert(i, "Default", true)
			if Settings.Strings.Ending == false then
				Status.Value = Time
			else
				Status.Value = Settings.Strings.Ending..Time
			end
			

			local RemainderBlue = {}
			local RemainderRed = {}

			for _, player in ipairs(Players:GetPlayers()) do
				if player.Team == Teams.Blue then
					if i == Settings.Round.RoundTime then
						table.insert(BlueCount,player.Name)
					end
					table.insert(RemainderBlue,player.Name)
				elseif player.Team == Teams.Red then
					if i == Settings.Round.RoundTime then
						table.insert(RedCount,player.Name)
					end
					table.insert(RemainderRed, player.Name)
				end	
			end
			
			if #RemainderRed == 0 and i == 0 then -- both teams lost
				print("Both lost")
				break
			end
			
			if #RemainderRed == 0 or i == 0 then -- blue won
				print("Blue won")
				break
			end
			
			if #RemainderBlue == 0 then -- red won
				print("Red won")
				break
			end
		end
	end
end

-- Connections
Values.InRound:GetPropertyChangedSignal("Value"):Connect(RoundChanged)

game.Players.PlayerAdded:Connect(function(player)
	script.Loading:Clone().Parent = player.PlayerGui

	player:GetPropertyChangedSignal("Team"):Connect(function()
		local character = player.character

		if player.Team == Teams.Red then
			Connections[player] = player.CharacterAdded:Connect(function()
				SetupPlayerForRedTeam(player)
			end)

			if character then
				SetupPlayerForRedTeam(player)
			end
		else
			Clean(Connections[player])
			--NOTE: A bit of a hack, maybe not worth it. nil and true evals to nil, and Clean can handle nil so it works.
			Clean(character and character.Head:FindFirstChild("KillBrick"))
		end
	end)

	player.CharacterAdded:Connect(function(character)
		character.Humanoid.Died:Connect(function()
			player.Team = Teams.Lobby
			player.CameraMode = Enum.CameraMode.Classic
			player.CameraMaxZoomDistance = 9
		end)
	end)
end)

-- Initilize
VoteSystem()
Jukebox:Round()

return Jukebox

The next step is to refactor the code to deal with some bad practices you’re making use of. Specifically getting rid of global state (variables) and bad choice of abstractions (I’ll explain).

[ThanksRoBama (Emma)]

Time to get rid of global variables:

You generally want to avoid this kind of “variables” part of a script. If there’s a lot of global variables, that’s a sign that you have global state. Google why that’s bad if you want to know more about that.

PlayerList only gets used in PickKiller, so make it local and give it a better name:

function PickKiller()
	local killerCandidates = {}
	for _, player in ipairs(game.Players:GetPlayers()) do  
		if player == previousKiller then continue end
		if not player.PlayerGui.Camera.Enabled then continue end --NOTE: I can't figure out why this check is present???

		table.insert(killerCandidates, player)
	end
	
	assert(#killerCandidates >= 1, "Cannot choose a killer (not enough players who aren't the previous killler)!")
	return PickRandom(killerCandidates)
end

Same with these 4 vars, although it’s a bit more subtle figuring it out for some:

It requires changing PickRandomMap to

function PickRandomMap() --Actually self explanatory :P
	return PickRandom(Maps)
end

And then changing the code that depends on randomMap being set to just using the return value instead. It’s a lot of places, e.g.

You’re doing some OOP-like stuff for the “Jukebox” table, but it doesn’t get used in any way that matters in this script. If no other script uses it either, I’d remove it and just use a Round function. I can tell that no other script requires it because Jukebox:Round is an infinite while loop, so you’d have to use task.spawn when you require it or something, which… yeah don’t there’s a better way if you’re really doing that xD

All this means it shouldn’t be a ModuleScript but just a normal script.

You have some function calls at the bottom of the script, I usually move them to an Init function at the top and then call that at the bottom, that way you can see what happens when the script runs at the top which is better IMO:

Replace this

with

I’d take the infinite while loop out of Round and put it in Init, that way it’s a function that “does a round” instead which makes more sense IMO, and it’s easier to see from the top of the script how the game flows (repeating rounds forever).

Then I’d put more of the logic into functions, e.g.

function DetermineWinningTeams(timeLeft: number):  {Team}
	local blueLeft = Teams.Blue:GetPlayers()
	local redLeft = Teams.Red:GetPlayers()
	
	if #redLeft == 0 or timeLeft == 0 then
		return {Teams.Blue}
	end
	
	if #blueLeft == 0 then
		return {Teams.Red}
	end

	if #redLeft == 0 and timeLeft == 0 then -- Time ran out *AND* all red dead?!
		return {Teams.Blue, Teams.Red}
	end

	return {}
end

The next changes I’d make are to the use of event-based programming, so when to use events and when not to use events. I would not do this:

Uploading: image.png(1)…

Because it makes the control flow bounce all over the place and you can’t have good names for things that happen. Plus something outside the script could potentially trigger it, which is bad because it’s hard to reason about. I’d replace RoundChanged with two separate functions, one for each case of “InRound.Value”, and just call those instead.

Now that things are getting better, it looks to me like the CanBeVoted stuff is just about picking some number of random things from a list of things that are not the same thing. That can be done without this kind of confusing global state, like this:

function PickSomeRandomly<T>(items: {T}, count: number): T
	assert(count <= #items, `Cannot pick {count} items out of only {#items}!`)
	local remaining = table.clone(items)
	local picked = {}
	for _ = 1, count do
		local roll = math.random(1, #remaining)
		table.insert(picked, table.remove(remaining, roll))	
	end
	return picked
end

Welp, I’m out of time to spend on this although there’s still a lot to be improved. The last version is here:

Summary

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

local Remotes = ReplicatedStorage.Remotes.Classic
local Values = ReplicatedStorage.Values
local Voting = game.Workspace.Lobby.Voting

local Maps = ServerStorage.Maps:GetChildren()
local Choices = Voting:GetChildren()

local TimeFormat = require(ReplicatedStorage.Modules.TimeFormat)

local Settings = {
	Round = {
		Interval = 15,
		RoundTime = 125
	},
	Strings = {
		NotEnoughPlayers = "Waiting for more players...",
		Starting = "Starting in: ",
	}
}

local Teams = {
	Red = game.Teams.Killer,
	Blue = game.Teams.Runner,
	Lobby = game.Teams.Lobby
}

local Status = Values.Status

-- Declarations
local previousKiller
local currentMap

local Connections = {}

function Init()
	StartMapVote()
	while true do
		RunRound()
	end
end

function PickRandom<T>(items: {T}): T
	return items[math.random(1, #items)]	
end

function PickSomeRandomly<T>(items: {T}, count: number): T
	assert(count <= #items, `Cannot pick {count} items out of only {#items}!`)
	local remaining = table.clone(items)
	local picked = {}
	for _ = 1, count do
		local roll = math.random(1, #remaining)
		table.insert(picked, table.remove(remaining, roll))	
	end
	return picked
end

function PickKiller()
	local killerCandidates = {}
	for _, player in ipairs(game.Players:GetPlayers()) do  
		if player == previousKiller then continue end
		if not player.PlayerGui.Camera.Enabled then continue end --NOTE: I can't figure out why this check is present???

		table.insert(killerCandidates, player)
	end
	
	assert(#killerCandidates >= 1, "Cannot choose a killer (not enough players who aren't the previous killler)!")
	return PickRandom(killerCandidates)
end

function Clean(item: nil|Instance|RBXScriptConnection)
	if typeof(item) == nil then
		return
	elseif typeof(item) == "RBXScriptConnection" then
		if not item.Connected then return end
		item:Disconnect()
	elseif typeof(item) == "Instance" then
		Debris:AddItem(item, 0)
	else
		error(`Cannot clean item of unknown type '{typeof(item)}'`)
	end
end

function GiveKillBrick(player: Player): Part
	local character = player.Character
	assert(character, `Cannot give kill brick to player without a character ({player.DisplayName})!`)

	local killBrick = ServerStorage.Assets.Red.KillBrick:Clone()
	killBrick:PivotTo(character.Head:GetPivot() + Vector3.new(0, 0, -2))
	killBrick.Orientation = Vector3.new(90, 0, 0) --TODO: Just modify the pivot of the killBrick so you don't need this. Or just swap the size's axes.
	
	killBrick.Weld.Part1 = character.Head --NOTE: No need to set it up in code, just have the weld as part of the prefab.
	killBrick.Parent = character.Head

	return killBrick
end

function SetupPlayerForRedTeam(player: Player)
	local killBrick = GiveKillBrick(player)
	killBrick.IsInUse.Value = true --TODO: Is this actually used? Otherwise remove

	player.CameraMode = "LockFirstPerson"
end

function StartMapVote()
	local mapsUpForVote = PickSomeRandomly(Maps, #Choices)
	for i, choice in ipairs(Choices) do
		local mapChoice = mapsUpForVote[i]

		choice.label.SurfaceGui.TextLabel.Text = mapChoice.Name
		choice.Image.SurfaceGui.ImageLabel.Image = mapChoice.Image.Value
	end
end

function SetupRound()
	local Choice1Votes = #Voting.Choice1.button.Votes:GetChildren()
	local Choice2Votes = #Voting.Choice2.button.Votes:GetChildren()
	local Choice3Votes = #Voting.Choice3.button.Votes:GetChildren()
	
	local chosenMapName
	if Choice1Votes >= Choice2Votes and Choice1Votes >= Choice3Votes then
		chosenMapName = Voting.Choice1.label.SurfaceGui.TextLabel.Text
	elseif Choice2Votes >= Choice1Votes and Choice2Votes >= Choice3Votes then
		chosenMapName = Voting.Choice2.label.SurfaceGui.TextLabel.Text
	else
		chosenMapName = Voting.Choice3.label.SurfaceGui.TextLabel.Text
	end
	
	Status.Value = "Map: " .. chosenMapName
	
	local map
	for _, _map in ipairs(Maps) do
		if chosenMapName ~= _map.Name then continue end
		map = _map:Clone()
		break
	end	

	currentMap = map
	map.Parent = game.Workspace
	
	for _, choice in ipairs(Choices) do
		choice.button.Votes:ClearAllChildren()
		Remotes.VoteReset:FireAllClients(choice.button)

		--NOTE: Shouldn't the voting GUI be made invisible? In that case there's no reason to reset it like this
		choice.label.SurfaceGui.TextLabel.Text = " "
		choice.Image.SurfaceGui.ImageLabel.Image = " "
	end
	
	local killer = PickKiller()
	--NOTE: No "if killer then" check needed, since PickKiller returns a player or errors
	
	previousKiller = killer

	local RedSpawns = map.RedSpawns:GetChildren()
	local BlueSpawns = map.BlueSpawns:GetChildren()

	for _, player in ipairs(game.Players:GetChildren()) do
		local character = player.character
		local rootPart = character:WaitForChild("HumanoidRootPart")

		-- Pick a random spawn from each team
		local playerSpawn
		local randomRedSpawn = PickRandom(RedSpawns)
		local randomBlueSpawn = PickRandom(BlueSpawns)

		--Misc. setup for each player
		player.CameraMaxZoomDistance = 9

		-- Per-team setup for each player
		if player == killer then
			player.Team = Teams.Red
			player.CameraMode = Enum.CameraMode.LockFirstPerson
			playerSpawn = randomRedSpawn
		else
			player.Team = Teams.Blue
			player.CameraMode = Enum.CameraMode.Classic
			playerSpawn = randomBlueSpawn
		end
		rootPart.CFrame = playerSpawn.CFrame + Vector3.new(math.random(1, 3), math.random(1, 3), math.random(1, 3))
	end
end

function CleanupRound()
	for _, player in ipairs(Players:GetPlayers()) do
		local character = player.character or player.CharacterAdded:Wait()
		local rootPart = character:FindFirstChild("HumanoidRootPart")
		rootPart.CFrame = workspace.Lobby.lobbySpawn.CFrame + Vector3.new(0, 5, 0)
		player.Team = Teams.Lobby

		Clean(character.Head:FindFirstChild("Killbrick"))
	end

	-- voting system
	Clean(currentMap)

	StartMapVote()
end

function DetermineWinningTeams(timeLeft: number): {Team}
	local blueLeft = Teams.Blue:GetPlayers()
	local redLeft = Teams.Red:GetPlayers()
	
	if #redLeft == 0 or timeLeft == 0 then
		return {Teams.Blue}
	end
	
	if #blueLeft == 0 then
		return {Teams.Red}
	end

	if #redLeft == 0 and timeLeft == 0 then -- Time ran out *AND* all red dead?!
		return {Teams.Blue, Teams.Red}
	end

	return {}
end

function RunRound()
	--Await 1 or more players --NOTE: Shouldn't there be at least 2 players though???
	repeat 
		Status.Value = Settings.Strings.NotEnoughPlayers
		task.wait(1)
	until #Players:GetChildren() >= 1

	--Countdown 
	for i = Settings.Round.Interval, 0, -1 do --NOTE: Probably rename Interval to Countdown? Unless I'm misunderstanding
		Status.Value = Settings.Strings.Starting .. i 
		task.wait(1)
	end
	
	--Round in progress
	SetupRound()

	local winners
	for timeLeft = Settings.Round.RoundTime, 0, -1 do
		--NOTE: Deleted some config stuff that seemed pretty unnecessary, it's up to you though
		Status.Value = TimeFormat:Convert(timeLeft, "Default", true)
		
		--Determine winners
		winners = DetermineWinningTeams(timeLeft)
		if #winners > 0 then break end
		
		task.wait(1)
	end

	--Deal with winners
	if #winners == 2 then
		print("It's a tie!")
	elseif #winners == 1 then
		print(`{winners[1].Name} team wins!`)
	end

	CleanupRound()
end

game.Players.PlayerAdded:Connect(function(player)
	script.Loading:Clone().Parent = player.PlayerGui

	player:GetPropertyChangedSignal("Team"):Connect(function()
		local character = player.character

		if player.Team == Teams.Red then
			Connections[player] = player.CharacterAdded:Connect(function()
				SetupPlayerForRedTeam(player)
			end)

			if character then
				SetupPlayerForRedTeam(player)
			end
		else
			Clean(Connections[player])
			--NOTE: A bit of a hack, maybe not worth it. nil and true evals to nil, and Clean can handle nil so it works.
			Clean(character and character.Head:FindFirstChild("KillBrick"))
		end
	end)

	player.CharacterAdded:Connect(function(character)
		character.Humanoid.Died:Connect(function()
			player.Team = Teams.Lobby
			player.CameraMode = Enum.CameraMode.Classic
			player.CameraMaxZoomDistance = 9
		end)
	end)
end)

Init()

I can’t guarantee that it works exactly as the original script, but if it has errors I’m sure you can implement the changes I suggested in your own code and make sure it works properly

[Remingling]

Thank you so so much for this. I need to learn to clean my code more so if there is any videos you think I should watch please comment them

PS: I put it to 1 player for testing