How do I fix this mess of a script

Help and Feedback Scripting Support
#1
Script 
local status = game:GetService("ReplicatedStorage"):WaitForChild("Status")
local serverstorage = game:GetService("ServerStorage")
local inround = game.ReplicatedStorage:WaitForChild("InRound")
local intermissionlength = 10
local increment = 1
local goal = 0
local roundlength = 120
local highest
local teams = game:GetService("Teams")
local playing = teams:WaitForChild("Playing")
local lobby = teams:WaitForChild("Lobby")
local playerservice = game.Players
local obj
local deathmatch = false
local slippery
local won = false
local eventlength = 30
function resetround()
	roundlength = 120
end
function resetintermission()
	intermissionlength = 10
end
function resetevent()
	eventlength = 30
end
function awardcoins(plr, input)
	print(plr.Name)
	print(plr.ClassName)
	plr:WaitForChild("leaderstats"):WaitForChild("Coins").Value += input
end
function awardlevels(plr, input)
	plr:WaitForChild("Levels"):WaitForChild("Exp").Value += input
end
local clone
local clone1
inround.Changed:Connect(function()
	print("yay it chhanged")
	if inround.Value == true and #playerservice:GetChildren() > 1 then
		for i,v in pairs(playerservice:GetChildren()) do
			local char = v.Character or v:LoadCharacter()
			if v.Team == "Playing" then
				print("erm what the sigma im looking at u like what the ligma")
			else
				print("errr")
				char.HumanoidRootPart.CFrame = workspace.Goal.CFrame
				v.Team = playing
			end

		end
		while true do
			task.wait(1)
			print("okaaa 52")
			if #playerservice:GetChildren() <= 1 then
				deathmatch = false
				print("okaaa 55")
				status.Value = "Not enough players to start the game!"
				break
			end
			if inround.Value == false then
				print("okaaa 60")
				break
			end
			eventlength -=1
			if deathmatch == true then
				print("okaaa 65")
				status.Value = "Death match ends in: ".. roundlength .. " seconds"
			else
				print("okaaa 68")
				status.Value = "Next event in: ".. eventlength .. " seconds"
				if eventlength == goal then
					if clone ~= nil then
						print("okaaa 72")
						clone:Destroy()
					end
					if clone1 ~= nil then
						print("okaaa 76")
						clone1:Destroy()
					end
					local random = math.random(1,2)
					if random == 1 then
						local lowgrav = coroutine.create(function()
							for i,v in pairs(game.Players:GetChildren()) do
								local char = v.Character or v.CharacterAdded:Wait()
								print("okaaa 84")
								if v == nil then
								else
									char:WaitForChild("Humanoid").JumpPower = 75
								end
								task.wait(eventlength)
								if v == nil then
								else
									char:WaitForChild("Humanoid").JumpPower = 50
								end
							end
						end)
						coroutine.resume(lowgrav)
						status.Value = "Low gravity"
					elseif random == 2 then
						 slippery = coroutine.create(function()
							print("okaaa 100")
							workspace.Hill.CustomPhysicalProperties.FrictionWeight = 100
							task.wait(eventlength)
							workspace.Hill.CustomPhysicalProperties.FrictionWeight = 50
						end)
					coroutine.resume(slippery)
					status.Value = "The hill is now reaaallllyyy slippery"
					end
					resetevent()
				end
			end
			roundlength -= increment
			if roundlength == 0 then
				local HighestVal, HighestPlayer
				print("okaaa 114")
				for _, player in game.Players:GetPlayers() do
					if not HighestVal or player.Character.Tag.Value > HighestVal then
						HighestVal = player.Character.Tag.Value
						HighestPlayer = player
					end
				end

				status.Value = "The winner is " .. HighestPlayer.Name .." with a time of " ..math.round(HighestVal).. " seconds!"
				print("okaaa 123")
				print("The winner is" .. HighestPlayer.Name .." with " ..math.round(HighestVal).. " time!")
						awardcoins(game:GetService("Players"):FindFirstChild(HighestPlayer.Name), 150)
				print("okaaa 126")
						deathmatch = false
						inround.Value = false
						task.wait(3)
						resetevent()
						resetround()
				print("okaaa 132")

			end
			if roundlength == goal then
				print("okaa 136a")
				if clone ~= nil then
					print("okaaa 138")
					clone:Destroy()
				end
				if clone1 ~= nil then
					print("okaaa 142")
					clone1:Destroy()
				end
				print("okaaa 145" )
				inround.Value = false
				resetround()
				break
			end
		end
	elseif inround.Value == false and #playerservice:GetChildren() > 1 then
		print("okaaa LINE  152")
		for i,v in pairs(playerservice:GetChildren()) do
			print("okaaa LINE  154")
			print(v.Team)
			if v.Team == playing then
				local char =  v:LoadCharacter() or v.Character
				print("Playing!!!?" .. char.Name)
				print("don't do anythin gpls")
			elseif v.Team == lobby then
				local char =  v:LoadCharacter() or v.Character
				print("lobby?" .. char.Name)
				print("okay LINE  163")
				print(char.Name)
				print(char.HumanoidRootPart.Position)
				char:WaitForChild("HumanoidRootPart").CFrame = workspace.Spectator.CFrame
				print(char.HumanoidRootPart.Position)
				v.Team = lobby
				
			end
		end
		while true do
			if #playerservice:GetChildren() <= 1 then
				print("okaaa LINE 208 174")
				status.Value = "Not enough players to start the game!"
				resetintermission()
				resetround()
				resetevent()
				break
			end
			task.wait(1)						
			print("okaaa LINE 182")
			status.Value = "Intermission: ".. intermissionlength .. " seconds"
			intermissionlength -= increment
			if intermissionlength == goal then
				inround.Value = true
				print("okaaa LINE 187")
				intermissionlength = 10
				break
			end
		end
	end
end)
game.Players.PlayerAdded:Connect(function()
	if #playerservice:GetChildren() <= 1 then
		print("okaaa LINE 196")
		status.Value = "Not enough players to start the game!"
		resetintermission()
		resetround()
		resetevent()
	else	
		print("okaaa LINE 202")
	inround.Value = false
	end
end)
game.Players.PlayerRemoving:Connect(function()
	if #playerservice:GetChildren() <= 1 then
		print("okaaa LINE 208")
		resetintermission()
		resetround()
		status.Value = "Not enough players to start the game!"
	end
end)

Yeah I’ve made 4 posts within 2 days of problems with this script, and I encountered another problem.
But I realized I think I need to just tidy this script up completely as it’s an absolute mess, the way I wrote is horrible and if somebody could tell me how I could not make it such a mess it’d be much apperciated

1 Like
#2

This really is a mess, it would require an antique clock repairer to even begin to attempt to fix the problem. The only thing I could recommend is to (if possible) finish the script so it functions, and then start over completely since you’ll know what the script needs to work.

You use break a lot. It might be better to encase your script in the if then, rather than use breaks everywhere. But that’s just my personal opinion. It can either make it cleaner or even worse depending on how you use it.

Your functions are unorganized, I’m seeing many random interjections in between functions. Such as eventlength and roundlength, is there any reason to change their values in between functions? It’s better to place them at the top or bottom of your script unless they’re in between functions for any actual reason.

Something I do to keep my scripts clean is use ModuleScripts. But that might not always fit, and it doesn’t look like it would fit what you’re trying to do. But it is still good advice for your other code nonetheless.

Some other criticism:
local random = math.random(1,2)
if random == 1 then
Why make a variable? It’s not being used anywhere else, unless I missed that.
It’s better to simply have it like this:
if math.random(1,2) == 1 then

I think using more functions might help you. You have a bunch of prints just below line 114, it could be tidied up with a function instead.

Lastly, use comments, as a reminder of what each function does. Try keeping it clean as if you had a partner who would have to try and understand your code later on.

Some of my takes might be wrong, but I still hope this is of some help to you.

1 Like
#3

1- use functions/modules
2-use comments
3-donot make all the variable names lowercase as it makes it harder to be read quickly (atleast for me)

function hellothisfunctiondoessomething()

end

function helloThisFunctionDoesSomething()

end

local verycoolvariable
local veryCoolVariable

4-many print statements everywhere that seems useless, that will not just make the code unreadable but will flood the console with useless stuff and you will not be able to find useful stuff easily
example

5- use for strings instead of many …

status.Value = "The winner is " .. HighestPlayer.Name .." with a time of " ..math.round(HighestVal).. " seconds!"


-- this is probably more readable
status.Value = `The Winner is {HighestPlayer.Name} with a time of {math.round(HighestVal)} seconds!`

6- make an order when naming variables
like services first and variables second etc…

7- use scoping

do
local scopeVariable = 5
code...
end

do
local scopeVariable = 5
code...
end

8- define parameter types in functions

--do
function awardlevels(plr :Player, input :Number)
	plr:WaitForChild("Levels"):WaitForChild("Exp").Value += input
end
--instead of
function awardlevels(plr, input)
	plr:WaitForChild("Levels"):WaitForChild("Exp").Value += input
end
#4

Adding to this post, here’s an example of how I tidied a horribly rushed function.

gm.notification = function(opt, title, body)
	if opt ~= nil then
		task.spawn(function()
			local x = gm.frz.design._eventtemplate:Clone()
			x.Text = body
			x.Parent = opt.PlayerGui.UI.Open
			wait(8)
			for i = 1, 100 do
				x.BackgroundTransparency += 0.01
				x.TextTransparency += 0.01
				wait(0.01)
			end
			x:Destroy()
		end)
	else
		for index, opt in pairs(game.Players:GetChildren()) do
			task.spawn(function()
				local x = gm.frz.design._eventtemplate:Clone()
				x.Text = body
				x.Parent = opt.PlayerGui.UI.Open
				wait(8)
				for i = 1, 100 do
					x.BackgroundTransparency += 0.01
					x.TextTransparency += 0.01
					wait(0.01)
				end
				x:Destroy()
			end)
		end
	end
end

Turned into:

gm.notification_handle = function(plr, title, body) --// plr: Player obj. title: Notification title. body: Notification text. Internal.
	task.spawn(function()
		local template = gm.template_notification:Clone()
		template.Text = body
		template.Parent = plr.PlayerGui.UI.Open.notifications
		wait(8)
		for i = 1, 100 do
			template.BackgroundTransparency += 0.01
			template.TextTransparency += 0.01
			wait(0.01)
		end
		template:Destroy()
	end)
end

gm.notification = function(opt, title, body) --// opt: Player obj OR nil. title: Notification title. body: Notification text. External. 
	if opt ~= nil then
		gm.notification_handle(opt, title, body)
	else
		for index, opt in pairs(game.Players:GetChildren()) do
			gm.notification_handle(opt, title, body)
		end
	end
end

Hopefully this is of some more help.

#5

Hey guys, wondering if this is any better?

Crime to humanity 
local replicatedstorage = game:GetService("ReplicatedStorage")
local players = game:GetService("Players")
local teams = game:GetService("Teams")
local playing = teams.Playing
local lobby = teams.Lobby
local status = replicatedstorage:WaitForChild("Status")
local rstatus = replicatedstorage:WaitForChild("RoundStatus")
local inround = replicatedstorage:WaitForChild("InRound")
local intermissionlength = 15
local roundlength = 120
local evenlength = 30
local intermissionbool = false
function intermission()
	intermissionbool = true
while true do
		task.wait(1)
		if intermissionbool == false then
			break
		end
	intermissionlength -=1
	status.Value = "Intermission: " .. intermissionlength
	if intermissionlength == 0 then
		intermissionbool = false
		inround.Value = true
	intermissionlength = 15
	break
	end
end
end
function event(char, hill)
	local lowgrav = coroutine.create(function()
		char.Humanoid.JumpPower = 100
		status.Value = "Everybody has low gravity now!"
		task.wait(evenlength)
		char.Humanoid.JumpPower = 50
	end)
	local slippery = coroutine.create(function()
		hill.CustomPhysicalProperties.FrictionWeight = 100
		status.Value = "Uh oh! The hill has just become extra slippery!"
		task.wait(evenlength)
		hill.CustomPhysicalProperties.FrictionWeight = 50
	end)
		if math.random(1,2) == 1 then
		coroutine.resume(lowgrav)
		if math.random(1,2) == 2 then
			coroutine.resume(slippery)
		end
	end
end
function findchar()
	for i,v in pairs(players:GetChildren()) do
		local char = v.Character or v.CharacterAdded:Wait()
		return char
	end
end

function teleport(char, location)
	char.HumanoidRootPart.Position = location
end
function findwinner()
		local HighestVal, HighestPlayer
		for _, player in game.Players:GetPlayers() do
			if not HighestVal or player.Character.Tag.Value > HighestVal then
				HighestVal = player.Character.Tag.Value
				HighestPlayer = player
				return HighestPlayer
			end
		end
end
function round()
	for i,v in pairs(players:GetChildren()) do
		v.Team = playing
	end
	for i,v in pairs(players:GetChildren()) do
		teleport(v.Character, workspace.Goal.Position)
	end
	status.Value = "Round has started!"
	while true do
		task.wait(1)
		roundlength -=1
		evenlength -=1
		rstatus.Value = "Round ending in " .. roundlength .. " seconds"
		status.Value = "Random event happening in: " .. evenlength .. " seconds"
		if evenlength == 0 then
			findchar()
			local char = findchar()
			event(char, workspace.Hill)
			evenlength = 30
		end
		if roundlength == 0 or inround.Value == false then
			if #game.Players:GetChildren() <= 1 then
				for i,v in pairs(players:GetChildren()) do
					v.Team = lobby
				end
				for i,v in pairs(players:GetChildren()) do
					teleport(v.Character, workspace.Spectator.Position)
				end
				status.Value = "Not enough players to start!"
				rstatus.Value = "Not in round"
				intermissionlength = 15
				intermissionbool = false
				roundlength = 120
				evenlength = 30
				break				
			end
			findwinner()
			local winner = findwinner()
			players:FindFirstChild(winner.Name).leaderstats.Coins.Value +=150	
			rstatus.Value = "Not in round"
			roundlength = 120
			inround.Value = false
			intermission()
			for i,v in pairs(players:GetChildren()) do
				v.Team = lobby
			end
			for i,v in pairs(players:GetChildren()) do
				teleport(v.Character, workspace.Spectator.Position)
			end
			break
		end
	end
	
	
end
players.PlayerAdded:Connect(function(plr)
	if #game.Players:GetChildren() <= 1 then
		status.Value = "Not enough players to start!"
		rstatus.Value = "Not in round"
		intermissionlength = 15
		intermissionbool = false
		roundlength = 120
		evenlength = 30
	elseif #game.Players:GetChildren() > 1 and inround.Value == false then
		intermission()
	elseif #game.Players:GetChildren() > 1 and inround.Value == true then
		plr.Team = playing
		end
end)
game.Players.PlayerRemoving:Connect(function(plr)
	if #game.Players:GetChildren() <= 1 then
		status.Value = "Not enough players to start!"
		rstatus.Value = "Not in round"
		inround.Value = false
		intermissionbool = false
		intermissionlength = 15
		roundlength = 120
		evenlength = 30
		end
end)
inround.Changed:Connect(function()
	if inround.Value == true then
				round()
	elseif inround.Value == false then	
	end
end)

It’s far from perfect but it might be better

#6

I agree with everyone else above, plus the fact that the styling and indentation is burning my eyes. I see a LOT of inconsistent amounts of tabs and new lines. Studio already tells you how to indent your lines, I’m surprised that it looks like this.

I like to make my variables, object names and etc. PascalCase to match with Roblox, and it looks a lot neater. For example, you can change plr to Player, inround to InRound, and i,v in the for loops to Index, Player. I see that you don’t even use the index, so you can replace it with the placeholder variable _ for _, Player. You can also do camelCase as aforementioned by someone else.

Speaking of the for loops, why are you using Players:GetChildren? It’s advised to use Players:GetPlayers instead.

You have multiple repeated for loops that unnecessarily loop over the same table twice? You can just move the body of the second loop in the first one, and I don’t think it will change anything.

findchar()
local char = findchar()

What is the purpose of calling findchar twice? Not to mention the actual function is flawed, it will only return the first player’s character. Maybe add a callback function instead of returning.

You have 7 lines that are repeated 3 times across the script with no difference, the one that tells you there are not enough players. You can just move that into a function and call it.

if inround.Value == true then
				round()
	elseif inround.Value == false then	
	end

…Why? Not only do you have an useless elseif condition that you can just replace with end (I can understand if you plan to use that part later, but you might want change it so it looks more readable), but you can remove == true since the IF condition checks for truthy values, basically doing the exact same thing for you. You can do this for the other conditions that check if booleans are true/false also:

--Old
if roundlength == 0 or inround.Value == false then

--New
if roundlength == 0 or (not inround.Value) then

Assuming you can understand and note my and everyone else’s improvements, you can fix this script by a ton.

#7

You have a strong grasp of focused logical flow. Suggest possibly adopting delegating specific actions to external function calls while maintaining that framework style of programming.

May have some of this script wrong. It’s more to answer the question you’re asking here.
Using a main logic chunk and delegating actions.

Script 
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ServerStorage = game:GetService("ServerStorage")
local Teams = game:GetService("Teams")
local Players = game:GetService("Players")

local Status = ReplicatedStorage:WaitForChild("Status")
local InRound = ReplicatedStorage:WaitForChild("InRound")
local Playing = Teams:WaitForChild("Playing")
local Lobby = Teams:WaitForChild("Lobby")

local INTERMISSION_LENGTH = 10
local ROUND_LENGTH = 120
local EVENT_LENGTH = 30

local function resetValues()
	INTERMISSION_LENGTH = 10
	ROUND_LENGTH = 120
	EVENT_LENGTH = 30
end

local function award(player, stat, amount)
	local statValue = player:WaitForChild(stat):WaitForChild("Value")
	statValue.Value += amount
end

local function teleportToPosition(players, position, team)
	for _, player in ipairs(players) do
		local character = player.Character or player:LoadCharacter()
		character:WaitForChild("HumanoidRootPart").CFrame = position
		player.Team = team
	end
end

local function handleEvent()
	local randomEvent = math.random(1, 2)

	if randomEvent == 1 then
		for _, player in ipairs(Players:GetPlayers()) do
			local character = player.Character or player.CharacterAdded:Wait()
			local humanoid = character:WaitForChild("Humanoid")
			humanoid.JumpPower = 75
		end

		task.wait(EVENT_LENGTH)

		for _, player in ipairs(Players:GetPlayers()) do
			local character = player.Character or player.CharacterAdded:Wait()
			local humanoid = character:WaitForChild("Humanoid")
			humanoid.JumpPower = 50
		end

		Status.Value = "Low gravity event ended."
	else
		local hill = workspace:WaitForChild("Hill")
		hill.CustomPhysicalProperties = PhysicalProperties.new(1, 0.3, 0.5, 100)

		task.wait(EVENT_LENGTH)

		hill.CustomPhysicalProperties = PhysicalProperties.new(1, 0.3, 0.5, 50)
		Status.Value = "Slippery hill event ended."
	end
end

local function declareWinner()
	local highestVal, winner

	for _, player in ipairs(Players:GetPlayers()) do
		local tagValue = player.Character:FindFirstChild("Tag") and player.Character.Tag.Value
		if tagValue and (not highestVal or tagValue > highestVal) then
			highestVal = tagValue
			winner = player
		end
	end

	if winner then
		Status.Value = string.format("The winner is %s with %d seconds!", winner.Name, math.round(highestVal))
		award(winner, "Coins", 150)
	else
		Status.Value = "No winner this round."
	end
end

InRound.Changed:Connect(function()
	if InRound.Value and #Players:GetPlayers() > 1 then
		teleportToPosition(Players:GetPlayers(), workspace.Goal.CFrame, Playing)

		while ROUND_LENGTH > 0 do
			task.wait(1)
			ROUND_LENGTH -= 1

			if #Players:GetPlayers() <= 1 then
				Status.Value = "Not enough players to continue!"
				break
			end

			if EVENT_LENGTH <= 0 then
				handleEvent()
				EVENT_LENGTH = 30
			end

			if ROUND_LENGTH == 0 then
				declareWinner()
				resetValues()
				InRound.Value = false
				break
			end

			EVENT_LENGTH -= 1
		end
	else
		teleportToPosition(Players:GetPlayers(), workspace.Spectator.CFrame, Lobby)

		while INTERMISSION_LENGTH > 0 do
			task.wait(1)
			INTERMISSION_LENGTH -= 1
			Status.Value = "Intermission: " .. INTERMISSION_LENGTH .. " seconds"

			if INTERMISSION_LENGTH == 0 then
				InRound.Value = true
				break
			end
		end
	end
end)

Players.PlayerAdded:Connect(function()
	if #Players:GetPlayers() <= 1 then
		resetValues()
		Status.Value = "Not enough players to start!"
	else
		InRound.Value = false
	end
end)

Players.PlayerRemoving:Connect(function()
	if #Players:GetPlayers() <= 1 then
		resetValues()
		Status.Value = "Not enough players to continue!"
	end
end)
#8

Going to drop some final thoughts before I move on.

You can’t really learn how to perfectly optimize and tidy your code within a day, same way Rome wasn’t built in a day.

It’ll come naturally, just make sure you focus on making the script readable, as if somebody else had to personally review it at a later time, best of luck.