Any Possible Optimizations?

Help and Feedback Code Review
, ,
1

Hello, this is one of the main modules in one of the server scripts inside my game although it’s gotten long. Is there any optimizations I could make to this module to shorten it? Any help is appreciated.

function module.SetupRound()
    TransitionEvent:FireAllClients()
    task.wait(1)
    
    local RandomMap = math.random(1, #Maps)
    local ChosenMap = Maps[RandomMap]:Clone()

    ChosenMap.Parent = game.Workspace
    
    RoundInfo.Status.Changed:Connect(function()
        if RoundInfo.Status.Value == "Intermission" and ChosenMap then
            ChosenMap:Destroy()
        end
    end)
    
    local PlayerSelection = Players:GetPlayers()
    local RandomPlayer = math.random(1, #PlayerSelection)
    local ChosenPlayer = PlayerSelection[RandomPlayer]
    
    local Character = Skins[ChosenPlayer.EquippedSkin.Value]:Clone()
    
    ChosenPlayer.Status.Value = "Ashes"
    ChosenPlayer.Character = Character
    Character.Parent = game.Workspace
    
    Animate.Parent = ChosenPlayer.Character
    
    for _, Player in ipairs(Players:GetPlayers()) do
        local PlayerSpawn = ChosenMap.Spawns:FindFirstChild("PlayerSpawn")
        local AshesSpawn = ChosenMap.Spawns:FindFirstChild("AshesSpawn")

        if Player.Status.Value ~= "Ashes" then
            Player.Status.Value = "Round"
        end

        if Player.Status.Value == "Round" then
            Player.Character:PivotTo(PlayerSpawn.CFrame)
        elseif Player.Status.Value == "Ashes" then
            Player.Character:PivotTo(AshesSpawn.CFrame)
        end
    end
end
1 Like
2

Try using this instead:

for _, Player in ipairs(Players:GetPlayers()) do
        local PlayerChar = Player.Character
        local PlayerStatus = Player:FindFirstChild("Status")
        local PlayerSpawn = ChosenMap.Spawns:FindFirstChild("PlayerSpawn")
        local AshesSpawn = ChosenMap.Spawns:FindFirstChild("AshesSpawn")

        if not PlayerStatus.Value == "Ashes" then
            PlayerStatus.Value = "Round"
        elseif PlayerStatus.Value == "Round" then
            PlayerChar:PivotTo(PlayerSpawn.CFrame)
        elseif PlayerStatus.Value == "Ashes" then
            PlayerChar:PivotTo(AshesSpawn.CFrame)
        end
    end
3

There are a lot of processes in the SetupRound method. This script can be much easier to understand by dividing all these processes in functions. This is how I personally like to write important functions:

function module.SetupRound()
        playTransitionAnimation()
        initRandomMap()
        spawnPlayers()
end

The name ‘module’ can be a bit more descriptive. Maybe you can call it ‘RoundManager’ or something?

4 Likes
4

You bind something to the changed event of RoundInfo.Status, but never unbind it. If you delete that RoundInfo object afterwards then it is fine, but if it persists over rounds it’s not good.

In the line

local PlayerSpawn = ChosenMap.Spawns:FindFirstChild("PlayerSpawn")

You should use WaitForChild. Some small segments of code are duplicated there, technically you could prevent that:

local function pivotPlayer(Player, spawnName)
  local spawn = ChosenMap.Spawns:WaitForChild(spawnName)
  Player.Character:PivotTo(spawn.CFrame)
end

local pivotList = {
  Round = function(plr) pivotPlayer(plr, "PlayerSpawn") end,
  Ashes = function(plr) pivotPlayer(plr, "AshesSpawn") end,
}

for _, Player in ipairs(Players:GetPlayers()) do
  local state = Player.Status
  state.Value = (state.Value ~= "Ashes") and "Round" or state.Value -- this is actually less readable but just to show
  pivotList[state.Value](Player)
end

But it may also just obscure things. The advantage of rewriting into fewer lines should be that it becomes more readable. Properly naming functions and variables can already help with that by itself. I didn’t really do a good job of that here, but maybe it helps to see alternative patterns like these for ideas.

Also you should make sure the character is there or wait for it / abort.

In general, I like to separate data, settings, names, etc, from logic and functions. Data goes into dictionaries, functions do something with the data. In this case the application may not be so great, but the names of statusses and the spawns belonging to them could be taken out of the main code, and defined in a table somewhere else. Usually when writing if-elseif-statements checking strings in a row, this is an option. It also prevents you from looking for many spawns when you are only using one of them, and this pattern would start to fit better with many spawns and statusses.

Anyway, just some food for thought, I’m not saying you should program it my way in this case. :slight_smile: Good luck!

Edit:

A more clear and straightforward implementation would in this case just be:

local statusToSpawnName = {
  Round = "PlayerSpawn",
  Ashes = "AshesSpawn",
}

local function pivotPlayer(Player, stateName)
  local spawnName = statusToSpawnName[stateName]
  local spawn = ChosenMap.Spawns:WaitForChild(spawnName)
  Player.Character:PivotTo(spawn.CFrame)
end

for _, Player in ipairs(Players:GetPlayers()) do
  local state = Player.Status
  state.Value = (state.Value == "Ashes") and state.Value or "Round" -- this is actually less readable but just to show
  pivotPlayer(Player, state.Value)
end

Lol, but in the end it’s not actually fewer lines and seems not that needed here. These are some alternative constructions of code that does the same thing, but ‘generalizing’ or ‘abstracting’ a bit more. If it were more complex, such patterns can save a lot of duplicated code. That’s why it’s often a good idea to abstract early and as a habit, to save yourself the trouble of rewriting everything later.

Because as a rule, projects and scripts tend to become more complex, and rarely less, unless you are extremely careful about it. :slight_smile:

2 Likes
5

Thanks for this, I’ve learned a lot from it.

1 Like