Scripting HelpersARCHIVE
A little piece of Roblox history. Preserved for everyone, one question at a time.Read about the archive →
All questions/Question #47136
✓ ANSWER ACCEPTED

Could you help me clean up this old script (250+ lines)?

Asked 35 views1 answers

I have this old script from 2016 that I created. At the time I was trying to create a game with it, but now that I know a ton more about scripting than I used to I was wondering if there is anyway I could organize this script better, It was one of the first long scripts I've ever written.

Ive built a game off this script and there are certain parts of the script I do not need anymore and I want to remove them so the server doesnt have to worry about those lines of code but when I remove some it breaks the rest, I've been trying to clean it up without breaking it.

But here it is, also maybe you could give me some advice on where I could improve.

--Made by Kofikingston9000

local roundtime = 60 * 4 local intermissiontime = 20

local datastore = game:GetService("DataStoreService"):GetDataStore("PlayerData") local serverstorage = game:GetService("ServerStorage") local replicatedstorage = game:GetService("ReplicatedStorage") local event = replicatedstorage:WaitForChild("RemoteEvent") local maps = serverstorage:WaitForChild("Maps") local mapholder = game.Workspace:WaitForChild("MapHolder") local statustag = replicatedstorage:WaitForChild("StatusTag") local timertag = replicatedstorage:WaitForChild("TimerTag")

local playerdata = {}

--set up score leaderboard --keep track of points --Drop sheriff net gun if bloxxed --make sure sheriff doesn't blox bystandars

function permanentsave(player) if player then local mydata = playerdata[player.userId] if mydata then datastore:SetAsync(player.userId, mydata) end end end

game.Players.PlayerRemoving:connect(function(player) if player then permanentsave(player) playerdata[player.userId] = nil end end)

game.OnClose = function() for i, v in pairs(playerdata) do datastore:SetAsync(i, v) end end

function findplayerbyid(userid) for _, player in pairs(game.Players:GetPlayers()) do if player and player.userId == userid then return player end end end

function newcharacter(bloxxedplayer, character) local humanoid = character:FindFirstChild("Humanoid") if humanoid then humanoid.Died:connect(function() end) end end

-- Officer and agent no kill good guys function newcharacter(bloxxedplayer, character) local humanoid = character:FindFirstChild("Humanoid") if humanoid then humanoid.Died:connect(function()

		local tag = humanoid:FindFirstChild("creator")
		if tag and tag.Value and tag.Value.Parent == game.Players then
			if tag.Value ~= culprit and bloxxedplayer ~= culprit then
				local wrongdoer = tag.Value
				if wrongdoer.Character and wrongdoer.Character.Parent then
					local wrongdoerhumanoid = wrongdoer.Character:FindFirstChild("Humanoid")
					if wrongdoerhumanoid then
						wrongdoerhumanoid.Health = 0
					end
				end
			end
		end 
	end)
end

end

while true do --load a random map statustag.Value = "Loading Map" timertag.Value = -1 mapholder:ClearAllChildren() wait(2) local allmaps = maps:GetChildren() local newmap = allmaps[math.random(1, #allmaps)]:clone() newmap.Parent = mapholder wait(2)

--wait for contestants
while true do
	wait(4)
	contestants = {}
	for _, player in pairs(game.Players:GetPlayers()) do
		if player and player.Character then
			local humanoid = player.Character:WaitForChild("Humanoid")
			if humanoid and humanoid.Health > 0 then
				table.insert(contestants, player)
			end
		end
	end
	if #contestants >= 3 then
		wait(5)
		break
	else
		statustag.Value = "Waiting for Players"
		timertag.Value = -1
	end
end

statustag.Value = "Match Time"
timertag.Value = roundtime

-- Choose culprit
culprit = contestants[math.random(1, #contestants)]

--choose sheriff
while true do
	randomplayer = contestants[math.random(1, #contestants)]
	if randomplayer ~= culprit then
		officer = randomplayer
		break
	end
end

-- Choose the Agent
while true do 
	randomplayer = contestants[math.random(1, #contestants)]
	if randomplayer ~= culprit and officer then
		agent = randomplayer
		break
	end
end	
--teleport contestants
local spawnsmodel = newmap:WaitForChild("Spawns")
local spawns = spawnsmodel:GetChildren()
for _, player in pairs(contestants) do
	if player and player.Character and #spawns > 0 then
		local torso = player.Character:WaitForChild("Torso")
		local humanoid = player.Character:WaitForChild("Humanoid")
		local spawnindex = math.random(1, #spawns)
		local spawn = spawns[spawnindex]
		if spawn and torso and humanoid then
			humanoid.Health = 100
			humanoid.WalkSpeed = 16
			table.remove(spawns, spawnindex)
			torso.CFrame = CFrame.new(spawn.Position + Vector3.new(0, 3, 0))
			
			local matchtag = Instance.new("StringValue")
			matchtag.Name = "MatchTag"
			matchtag.Parent = player.Character

			local backpack = player:FindFirstChild("Backpack")
			if backpack then
				if player == culprit then
					humanoid.WalkSpeed = 18
					local machete = serverstorage:WaitForChild("Machete"):clone()
					machete.Parent = backpack
					local lantern = serverstorage:WaitForChild("Lantern"):clone()
					lantern.Parent = backpack
					event:FireClient(player, "Class", "Culprit")
					
				elseif player == officer then
					local machete = serverstorage:WaitForChild("Machete"):clone()
					event:FireClient(player, "Class", "Officer")
					
				elseif player == agent then
					local machete = serverstorage:WaitForChild("Machete"):clone()
					event:FireClient(player, "Class", "Agent")
						
				else
					local lantern = serverstorage:WaitForChild("Lantern"):clone()
					lantern.Parent = backpack
					event:FireClient(player, "Class", "Victim")
				end
			end
		end
	end
end
spawnsmodel:remove()

--wait for round end conditions
local localtimer = roundtime
while localtimer > 0 do
	wait(1)
	localtimer = localtimer - 1
	timertag.Value = localtimer

	activecontestants = {}
	culpritactive = false
	for _, player in pairs(contestants) do
		if player then
			local character = player.Character
			if character then
				local matchtag = character:FindFirstChild("MatchTag")
				local humanoid = character:FindFirstChild("Humanoid")
				if matchtag and humanoid and humanoid.Health > 0 then
					if player == culprit then
						culpritactive = true
					end
					table.insert(activecontestants, player)
				end
			end
		end
	end
	if #activecontestants <= 1 or not culpritactive then
		break
	end
end

--sort game result
local gameresult = "PlayersWin"
if culpritactive then
	if #activecontestants >= 2 then
		event:FireAllClients("Result", "PlayersWin")
	else
		gameresult = "CulpritWin"
		event:FireAllClients("Result", "CulpritWin")
	end
end
for _, v in pairs(game.Workspace:GetChildren()) do
	if v then
		if v.Name == "Pickup" or v.className == "Hat" then
			v:remove()
		end
	end
end

--teleport plays back to lobby
local lobbyspawns = {}
for _, v in pairs(game.Workspace:WaitForChild("Lobby"):GetChildren()) do
	if v and v.Name == "SpawnLocation" then
		table.insert(lobbyspawns, v)
	end
end
for _, player in pairs(activecontestants) do
	if player then
		if player.Character then
			local humanoid = player.Character:FindFirstChild("Humanoid")
			if humanoid then
				humanoid:UnequipTools()
			end
		end

		local randomspawn = lobbyspawns[math.random(1, #lobbyspawns)]
		player.Character:MoveTo(randomspawn.Position)

		local backpack = player:FindFirstChild("Backpack")
		if backpack then
			backpack:ClearAllChildren()
		end
	end
end
culprit = nil

--intermission wait
statustag.Value = "Intermission"
timertag.Value = intermissiontime
local localtimer = intermissiontime
while localtimer > 0 do
	wait(1)
	localtimer = localtimer - 1
	timertag.Value = localtimer
end

end

I do realize the site doesnt like the script, some of it will be in a script box and others wont. Sorry :(

Anonymous

1 comment

0

If you "know a ton more about scripting", you should need us to organize it...

Anonymous#

1 answer

✓ Accepted answer

It's part of scripting to keep your code clean. Here are some tips:

  • Use proper indentation
  • Whenever you see duplicate code (with only minor differences), extract it into a function
  • Every bit of code that you could put a name to what it's doing, some recommend putting that into a function. ex, at the end you have a while loop to run the game logic. You might extract the code so that you have functions WaitForContestants, ChooseCulprit, ChooseSheriff, TeleportContestants, GivePlayerStartingTools, etc. Note that each of those Choose functions can merged - assign a 'type' for each person as you choose their role and don't assign a type to someone who already has one.
  • You can make your code more modular (and therefore easier to change) if your functions don't use global variables (or at least keep it to a minimum). Instead, pass in just the information they need to operate, and keep the function general. For instance, instead of ChooseCulprit, you might have ChooseRole(contestants, contestantRolesSoFar, desiredRole) -- it needn't use global variables, allowing you to change those global variables at any time
  • connect is deprecated, use Connect
  • Organizing concepts into objects (Object Oriented Programming) can make things easier to think about and/or use functions as variables to keep your code easy to read:
--Instead of "local backpack = player:FindFirstChild("Backpack")" onward, you could have:
local role = GetRoleFromPlayer(player)
if role then
    role:Initialize(player)
end
-- near top of script:
function Role(name, toolList)
    local self = {}
    function self:GenerateStarterTools(backpack)
        local tool
        for i = 1, #toolList do
            tool = serverstorage:WaitForChild(toolList[i]):Clone()
            tool.Parent = backpack
        end
    end
    function self:SetupPlayer(player) -- returns true if successful
        -- do nothing; this is here because Culprit needs to override it
        return true
    end
    function self:Initialize(player, event) -- returns true if successful
        local backpack = player:FindFirstChild("Backpack")
        if not backpack then return false end
        self:GenerateStarterTools(backpack)
        if not self:SetupPlayer(player) then return false end
        event:FireClient(player, "Class", name)
        return true
    end
    function self:GetType() return name end
    return self
end
function Culprit() -- Inherits from Role
    local self = Role("Culprit", {"Machete", "Lantern"})
    local baseSetupPlayer = self.SetupPlayer
    function self:SetupPlayer(player, event)
        if not baseInitialize(player) then return false end
        humanoid = player.Character:FindFirstChild("Humanoid")
        if not humanoid then return false end
        humanoid.WalkSpeed = 18
        return true
    end
    return self
end
--Note that there are other ways of inheriting from something with benefits/drawbacks; feel free to look it up
function Officer()
    return Role("Officer", {"Machete"})
end
function Agent()
    return Role("Agent", {"Machete"})
end
-- etc

--Maybe you use a dictionary to assign roles (remember to reset it before each match):
roles = {}
roles[player] = Agent() -- for example
--You can now do
roles[player]:Initialize(player)

--You could also rewrite Role a bit to receive 'player' as an argument, meaning you don't have to pass it in each time

When you use this style of programming, it can often be easier to make changes and it becomes more obvious what the code is doing.

Finally, note that some consider a script to be "clean" only when it's got tests for it (look up "unit testing" or "test driven development") -- it's not the easiest thing to do, but if the script is important, you can more easily verify that it's working (especially if you later change it and still need it to be working).

AnonymousPermalink

1 comment

0

Thanks man :) I shall clean my script up following these steps.

Anonymous#

This discussion is preserved as it was. New answers, comments, and votes are closed.