Scripting Helpers is winding down operations and is now read-only. More info→
Ad
Log in to vote
1

Is there anyway to make this code cleaner and shorter?

Asked by 5 years ago

I'm making a character creator, but I feel like it has too many else ifs. I was wondering if there was anything that I could use to clean this up and make it easier to read/edit:

wait(1)
local Player = game.Players.LocalPlayer
local Shirt = 0
local Pants = 0
----------------------------------------------------------------------------------------------------------------------------
script.Parent.Frame.ShirtButton.MouseButton1Click:Connect(function()
    if Shirt == 0 then
        Shirt = 1
        game.ReplicatedStorage.CharItems.Shirt1:Clone().Parent = workspace.You
    elseif Shirt == 1 then
        Shirt = 2
        workspace.You.Shirt1:Destroy()
        game.ReplicatedStorage.CharItems.Shirt2:Clone().Parent = workspace.You
    elseif Shirt == 2 then
        Shirt = 3
        workspace.You.Shirt2:Destroy()
        game.ReplicatedStorage.CharItems.Shirt3:Clone().Parent = workspace.You
    elseif Shirt == 3 then
        Shirt = 4
        workspace.You.Shirt3:Destroy()
        game.ReplicatedStorage.CharItems.Shirt4:Clone().Parent = workspace.You
    elseif Shirt == 4 then  
        workspace.You.Shirt4:Destroy()
        Shirt = 1
        game.ReplicatedStorage.CharItems.Shirt1:Clone().Parent = workspace.You
    end
end)
------------------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------------------------------
script.Parent.Frame.PantsButton.MouseButton1Click:Connect(function()
    if Pants == 0 then
        Pants = 1
        game.ReplicatedStorage.CharItems.Pants1:Clone().Parent = workspace.You
    elseif Pants == 1 then
        Pants = 2
        workspace.You.Pants1:Destroy()
        game.ReplicatedStorage.CharItems.Pants2:Clone().Parent = workspace.You
    elseif Pants == 2 then
        Pants = 3
        workspace.You.Pants2:Destroy()
        game.ReplicatedStorage.CharItems.Pants3:Clone().Parent = workspace.You
    elseif Pants == 3 then
        Pants = 4
        workspace.You.Pants3:Destroy()
        game.ReplicatedStorage.CharItems.Pants4:Clone().Parent = workspace.You
    elseif Pants == 4 then  
        workspace.You.Pants4:Destroy()
        Pants = 1
        game.ReplicatedStorage.CharItems.Pants1:Clone().Parent = workspace.You
    end
end)
------------------------------------------------------------------------------------------------------------

Thanks for the help

0
you simply cant DinozCreates 1070 — 5y
0
He can do it in a simple way. It's not impossible yHasteeD 1819 — 5y
0
oh i was making fun of the notorious "how can i shorten this code" post. i cant seem to find the link though. DinozCreates 1070 — 5y

1 answer

Log in to vote
2
Answered by 5 years ago
Edited 5 years ago

Improvements you can make to your code:

  • Use the modulus operator to automatically wrap around your counter variable (like Pants in your script) instead of using an if statement to check it. In case you didn't know, % is the modulus operator; it returns the remainder of a division. (You'll see it in use below.)
  • Use WaitForChild instead of a wait(1) at the top (better practice - the script becomes ready faster in most cases and if anything lags while loading then the script won't break)
  • Use variables to keep track of parents of commonly used folders/instances, like CharItems.
  • Index the parent using [] notation. Since obj.Child is the same as obj["Child"], we can access a child with a variable number at the end by doing obj["Name" .. num].

Code so far (I don't comment it because I have better code below):

local player = game.Players.LocalPlayer
local charItems = game.ReplicatedStorage:WaitForChild("CharItems")
local frame = script.Parent:WaitForChild("Frame")

local shirtNum = 0
local numShirts = 4
local shirt
frame:WaitForChild("ShirtButton").MouseButton1Click:Connect(function()
    if shirt then shirt:Destroy() end
    shirtNum = shirtNum % numShirts + 1
    shirt = charItems["Shirt" .. shirtNum]:Clone()
    shirt.Parent = player.Character
end)

local pantsNum = 0
local numPants = 4
local pants
frame:WaitForChild("PantsButton").MouseButton1Click:Connect(function()
    if pants then pants:Destroy() end
    pantsNum = pantsNum % numPants + 1
    pants = charItems["Pants" .. pantsNum]:Clone()
    pants.Parent = player.Character
end)

Notice how the pants and shirt code are very similar? You can use a single function to handle any clothing type. This is especially valuable if you might add in other clothing types - if you ever need to make a change to all of them, you only need to do it in one place rather than 2+! However, it may not be the best idea for your case if:

  • You intend on making clothing items work very differently in the future (ex if shirts started using a 2-part naming system, like "Shirt" .. theme .. num, whereas pants just kept using "Pants" .. num, or maybe one clothing type uses special permissions or checks that the others don't)
  • If you don't understand it (meaning you can't edit it) - though you can ask for further clarifications to fix this one =)

The function HandleClothing below does this, taking as parameter anything that is different between the types of clothing.

local player = game.Players.LocalPlayer
local charItems = game.ReplicatedStorage:WaitForChild("CharItems")
local frame = script.Parent:WaitForChild("Frame")

function HandleClothing(buttonName, charItemName, maxItems)
    local num = 0 -- which clothing number we gave out last
    local item -- clothing item we last gave out
    frame:WaitForChild(buttonName).MouseButton1Click:Connect(function()
        if item then item:Destroy() end -- destroy the last item we gave out
        num = num % maxItems + 1 -- ex, if 'num' is 4 and 'maxItems' is 4, 4 % 4 is 0, so 'num' becomes 4%4+1 == 1
        item = charItems[charItemName .. num]:Clone()
        item.Parent = player.Character
    end)
end
HandleClothing("ShirtButton", "Shirt", 4)
HandleClothing("PantsButton", "Pants", 4)

Other potential improvements:

  • You could have the HandleClothing function automatically detect the maximum number of items over time by using FindFirstChild (and setting max when it finds that a child doesn't exist), though this complicates the code a lot more just to save you from having to keep the number of clothing items up-to-date.
Ad

Answer this question