How can I improve my code

Help and Feedback Code Review
1

I want to see how I can make this code cleaner and what I can do to improve it

Round code 
local roundModule = {}

local replicatedStorage = game:GetService("ReplicatedStorage")
local serverStorage = game:GetService("ServerStorage")
local players = game:GetService("Players")

local lobbyFolder = workspace.Lobby

local roundTime = 5 -- seconds
local minPlayers = 1
local intermissionTime = 5 -- seconds
local gameEndTime = 3
local gamemodeVotingTime = 10

local startingPlayers = 0

local valuesFolder = replicatedStorage:WaitForChild("Values")
local remoteEvents = replicatedStorage:WaitForChild("RemoteEvents")

local winnersRemote = remoteEvents.Winners
local chosenGamemodeRemote = remoteEvents.ChosenGamemode

local status = valuesFolder.Status
local timeValue = valuesFolder.Time

local playersDead = {}
local playersAlive = {}

local roundRunning = true
local votingOngoing = false

local GridModule = require(serverStorage.Modules.Grid)
local gamemodeModule = require(serverStorage.Modules.Round.Gamemodes)

local chosenGamemode = nil
local chosenGameModeValue = valuesFolder.ChosenGamemode

-- GIVE REWARDS
-- FIX ANYTHING ELSE

local function hasEnoughPlayers()
	if #players:GetPlayers() >= minPlayers then
		return true
	else
		return false
	end
end

function teleportToLobby(player)
	if not player then
		for _, player in pairs(players:GetPlayers()) do
			if player.Character then
				player.Character:PivotTo(lobbyFolder.LobbyModel.Spawn.CFrame) 
			end
		end
	else
		if player.Character then
			player.Character:PivotTo(lobbyFolder.LobbyModel.Spawn.CFrame) 
		end
	end
end

function teleportToRound(player)
	local function findRandomSpawnPoint()
		local spawnPoints = {}
		for _, spawnPoint in pairs(workspace.Game:GetDescendants()) do
			if spawnPoint:IsA("BasePart") and spawnPoint.Name == "Spawn" then
				table.insert(spawnPoints, spawnPoint)
			end
		end
		return spawnPoints[math.random(1, #spawnPoints)]
	end
	
	if not player then
		for _, player in pairs(players:GetPlayers()) do
			if player.Character then
				player.Character:PivotTo(findRandomSpawnPoint().CFrame) 
			end
		end
	else
		if player.Character then
			player.Character:PivotTo(findRandomSpawnPoint().CFrame) 
		end
	end
end

function roundModule.Init()
	players.PlayerAdded:Connect(function(player)
		player.CharacterAdded:Connect(function()
			table.insert(playersDead, player)
			teleportToLobby(player)
			
			player.Character.Humanoid.Died:Connect(function()
				table.remove(playersAlive, table.find(playersAlive, player))
				table.insert(playersDead, player)
			end)
		end)
	end)

	players.PlayerRemoving:Connect(function(player)
		local index = table.find(playersAlive, player)
		if index then
			table.remove(playersAlive, index)
		end
	end)
end

function roundModule.lackOfPlayers()
	status.Value = "Not enough players"
	timeValue.Value = 0
end

function roundModule.Intermission()
	local graceTime = 3
	chosenGameModeValue.Value = ""
	
	for i = intermissionTime, 0, -1 do
		status.Value = "Intermission"
		timeValue.Value = i
		task.wait(1)
	end
	
	local votedClients = {}
	votingOngoing = true
	
	local gamemodesFolder = valuesFolder.Gamemodes
	
	local gamemodes = gamemodeModule.returnGamemode(3)
	
	local votingRemote = replicatedStorage.RemoteEvents.GamemodeVoting
	votingRemote:FireAllClients(gamemodes, gamemodeVotingTime, graceTime)
	
	gamemodesFolder:ClearAllChildren()
	
	for i,v in pairs(gamemodes) do
		local gamemodeValue = Instance.new("IntValue")
		gamemodeValue.Name = gamemodes[i].Name
		gamemodeValue.Parent = gamemodesFolder
	end
	
	local connection = votingRemote.OnServerEvent:Connect(function(player, gamemode)
		if not votingOngoing then
			warn("Voting has ended!")
			return
		end
		if not gamemodes[gamemode] then
			warn("Gamemode does not exist")
			return
		end
		if votedClients[player.Name] then
			warn("Changed vote")
			
			gamemodesFolder:FindFirstChild(votedClients[player.Name]).Value -= 1
			
			gamemodesFolder:FindFirstChild(gamemode).Value += 1
			
			votedClients[player.Name] = gamemodes[gamemode].Name
			return
		end
		votedClients[player.Name] = gamemodes[gamemode].Name
		
		gamemodesFolder:FindFirstChild(gamemode).Value += 1
	end)
	
	task.wait(gamemodeVotingTime)
	
	votingOngoing = false
	connection:Disconnect()
	
	local AllValues = gamemodesFolder:GetChildren()
	
	for i, v in pairs(AllValues) do
		AllValues[i] = v.Value
	end
	table.sort(AllValues)

	local HighestValue = AllValues[#AllValues]
	local LowestValue = AllValues[1]
	
	for i, v in pairs(gamemodesFolder:GetChildren()) do
		if v.Value == HighestValue then
			chosenGamemode = v.Name
			chosenGameModeValue.Value = v.Name
			break
		end
	end
	
	chosenGamemodeRemote:FireAllClients(chosenGamemode)
		
	task.wait(graceTime)
		
	for i = 5, 0, -1 do
		status.Value = "Round starting in"
		timeValue.Value = i
		task.wait(1)
	end
end

function roundModule.finishRound()
	task.wait(1) -- gives time for the gamemodes to kill the losing team, etc.
	
	local timeElapsed = roundTime - timeValue.Value
	if timeElapsed > roundTime/12 and #playersAlive > 0 then
		status.Value = "Game over"
		timeValue.Value = 0
			
		winnersRemote:FireAllClients(playersAlive) -- maybe include how long the game lasted?
			
		for _, player in pairs(playersAlive) do
			-- give rewards
		end
			
		table.clear(playersAlive)
		task.wait(gameEndTime)
		teleportToLobby()
		status.Value = "Cleaning up map"
		GridModule:CleanUp()
		startingPlayers = 0	
		return
	end
	
	status.Value = "No winners"
	-- no rewards
	timeValue.Value = 0
	table.clear(playersAlive)
	task.wait(gameEndTime)
	teleportToLobby()
	status.Value = "Cleaning up map"
	GridModule:CleanUp()
	startingPlayers = 0	
end

function roundModule.startRound()
	status.Value = "Generating Map"
	GridModule:GenerateGrid()
	table.clear(playersDead)
	status.Value = "Game"
	for _, player in pairs(players:GetPlayers()) do
		if player.Character then
			table.insert(playersAlive, player)
			teleportToRound(player)
			startingPlayers +=1
		end
	end
	print(chosenGamemode)
	local chosenGamemodeModule = gamemodeModule.chooseGamemode(chosenGamemode) 
	for i = roundTime, 0, -1 do
		timeValue.Value = i
		if #playersAlive < minPlayers then
			break
		end
		task.wait(1)
	end
end

function roundModule.Initalise()
	roundModule.Init()
	while true do
		local success, err = pcall(function()
			repeat
				task.wait(1)
				roundModule.lackOfPlayers()
			until hasEnoughPlayers()

			roundModule.Intermission()
			roundModule.startRound()
			roundModule.finishRound()
		end)

		if not success then
			warn(err)
			task.wait(1)
		end
	end
end


return roundModule
2

For a round based code I would personally try to keep the logic easy to read with good function naming etc but also keep functions and adjustable values in a separate module so you have like a module called ”Constants” where you store these adjustable values such as what you got ”roundtime” etc and make another module where you store all your functions like the teleport func.

The point is to keep the main script easy to read so its easier to add features later.

2 Likes
3

consider using types for parameters and --!strict

for example 
function teleportToLobby(player)

vs

function teleportToLobby(player: Player)

that would help you once you decide to extend your code and will start changing things.
Because in software development “clean” code refers to “code that is easy to read, understand, modify, and maintain by any developer, not just the person who wrote it”.

3 Likes
4
New Script 
local roundModule = {}

local replicatedStorage = game:GetService("ReplicatedStorage")
local players = game:GetService("Players")

local utillityModule = require(script.UtillityModule)
local mapGenerationModule = require(script.MapGeneration)
local gamemodeModule = require(script.Gamemodes)

local valuesFolder = replicatedStorage:WaitForChild("Values")
local remoteEvents = replicatedStorage:WaitForChild("RemoteEvents")

local winnersRemote = remoteEvents.Winners

local status = valuesFolder.Status
local timeValue = valuesFolder.Time

local playersDead = {}
local playersAlive = {}

local votingOngoing = false
local startingPlayers = 0
local chosenGamemode = nil

-- GIVE REWARDS

function roundModule.Init()
	players.PlayerAdded:Connect(function(player)
		player.CharacterAdded:Connect(function()
			table.insert(playersDead, player)
			utillityModule.teleportToLobby(player)
			
			player.Character.Humanoid.Died:Connect(function()
				table.remove(playersAlive, table.find(playersAlive, player))
				table.insert(playersDead, player)
			end)
		end)
	end)

	players.PlayerRemoving:Connect(function(player)
		local index = table.find(playersAlive, player)
		if index then
			table.remove(playersAlive, index)
		end
	end)
end

function roundModule.lackOfPlayers()
	status.Value = "Not enough players"
	timeValue.Value = 0
end

function roundModule.Intermission()
	local graceTime = 3
	chosenGameModeValue.Value = ""
	
	utillityModule.countDown("Intermission", utillityModule.intermissionTime)
	
	chosenGamemode = utillityModule.Voting()
		
	utillityModule.countDown("Round Starting In", 5)
end

function roundModule.finishRound()
	task.wait(1) -- gives time for the gamemodes to kill the losing team, etc.
	
	local timeElapsed = utillityModule.roundTime - timeValue.Value
	if timeElapsed > utillityModule.roundTime/12 and #playersAlive > 0 then
		status.Value = "Game over"
		timeValue.Value = 0
			
		winnersRemote:FireAllClients(playersAlive) -- maybe include how long the game lasted?
			
		for _, player in pairs(playersAlive) do
			-- give rewards
		end
		
		utillityModule.cleanUp()
		table.clear(playersAlive)
		startingPlayers = 0	
		return
	end
	
	utillityModule.cleanUp()
	table.clear(playersAlive)
	startingPlayers = 0	
end

function roundModule.startRound()
	status.Value = "Generating Map"
	mapGenerationModule:GenerateGrid()
	table.clear(playersDead)
	status.Value = "Game"
	
	for _, player in pairs(players:GetPlayers()) do
		if player.Character then
			table.insert(playersAlive, player)
			utillityModule.teleportToRound(player)
			startingPlayers +=1
		end
	end
	
	print(chosenGamemode)
	local chosenGamemodeModule = gamemodeModule.chooseGamemode(chosenGamemode) 
	for i = utillityModule.roundTime, 0, -1 do
		timeValue.Value = i
		if #playersAlive < utillityModule.minPlayers then
			break
		end
		task.wait(1)
	end
end

function roundModule.Initalise()
	roundModule.Init()
	while true do
		local success, err = pcall(function()
			repeat
				task.wait(1)
				roundModule.lackOfPlayers()
			until utillityModule.hasEnoughPlayers()

			roundModule.Intermission()
			roundModule.startRound()
			roundModule.finishRound()
		end)

		if not success then
			warn(err)
			task.wait(1)
		end
	end
end


return roundModule

I’ve managed to shorten it down by 150 lines of code thank you guys

5

personally, I would use attributes instead of using values as a instance as they are faster and uses less memory. Just keep in track on what you are setting and you should be good to go. If you want to detect If a value has been changed then do:

{instance}:GetAttributeChangedSignal({attribName}):Connect(function)
     --// code
end)
6

like as in using attributes for the status/time values?

7

yes. but honestly using the values are fine as is. it’s just worth noting that attributes do exist and can be used in place of them

2 Likes