What is wrong with this script?

Asked by

User#5689 -1

9 years ago

Please make your question title relevant to your question content. It should be a one-sentence summary in question form.

I asked this question before and I got the answer without having an error. This script is for selling shirt and pants, but with a try button included. Well, the try button is still not working when I clicked it. When I clicked it, it did nothing. There isn't any error so that went okay, but it is not letting me try them. This is the script:

dist = 10

function onClicked()
    local players = game.Players:GetChildren()
    for i = 1, #players do
    if (players[i].Character.Torso.Position - script.Parent.Position).magnitude < dist then
    local name = players[i].Name
    local clothing = players[i]:GetChildren()
    for i = 1, #clothing do
if clothing[i]:IsA("Clothing") then
    local f = game.Workspace:FindFirstChild(name)
    local s = f:FindFirstChild("Shirt")
    local p = f:FindFirstChild("Pants")
    local s1 = script.Parent.Parent.Parent.Clothes.Shirt:clone()
    local p1 = script.Parent.Parent.Parent.Clothes.Pants:clone()
    s.Parent = script.Parent.Parent.PlayerStorage
    p.Parent = script.Parent.Parent.PlayerStorage
    wait()
    s1.Parent = f
    p1.Parent = f
    wait(5)
    s1:remove()
    p1:remove()
    s.Parent = f
    p.Parent = f
end
end
    end
    end
end

script.Parent.ClickDetector.MouseClick:connect(onClicked)

This is once again terrible code (that AGAIN I have retyped a solution to fix) that has a LARGE number of issues. Address those **first**, then **debug**, THEN come back. BlueTaslem 18071 — 9y

1 answer

Answered by

BlueTaslem 18071

9 years ago

Tab your code correctly.
Use generic for loops: i is even reused twice! But it's not used anywhere except to index the thing your'e looping over.
MouseClick tells you playerWhoClicked. Don't use that loop. It's unnecessary.
Use meaningful variable names f is a character. s is shirt. p is pants

Miscellaneous: Use :Destroy() not :remove().

function onClicked(player)
    local name = player.Name
    local clothing = player:GetChildren()
    for _, cloth in pairs(clothing) do
        if cloth:IsA("Clothing") then
            local character = game.Workspace:FindFirstChild(name)
            local shirt = character:FindFirstChild("Shirt")
            local pants = character:FindFirstChild("Pants")
            local newShirt = script.Parent.Parent.Parent.Clothes.Shirt:clone()
            local newPants = script.Parent.Parent.Parent.Clothes.Pants:clone()
            shirt.Parent = script.Parent.Parent.PlayerStorage
            pants.Parent = script.Parent.Parent.PlayerStorage
            wait()
            newShirt.Parent = character
            newPants.Parent = character
            wait(5)
            newShirt:Destroy()
            newPants:Destroy()
            shirt.Parent = character
            pants.Parent = character
        end
    end
end

script.Parent.ClickDetector.MouseClick:connect(onClicked)

Issue 1: character is being defined strangely, and in a weird place. It's the player's character, so just define it as player.Character (this also makes name unnecessary).

Issue 2: clothing = player:GetChildren() is wrong. The player doesn't have clothing, its character does. Use clothing = character:GetChildren().

Issue 3: cloth is unused. You should use cloth, you should not figure out shirt and pants.

Issue 4: The character may not have a Shirt or Pants. This will cause errors, since you do things with these objects anyways.

Get rid of shirts and pants. Just use cloth instead.

Issue 5: You don't give them new clothes per cloth they are wearing -- you do it once -- so it should not be in the loop.

Issue 6: You are using just one PlayerStorage, which now won't work necessarily, since all players using the stand would share it. We need privates ones: Just make a new one as a local model: local storage = Instance.new("Model")

function onClicked(player)
    local character = player.Character
    local clothing = character:GetChildren()
    local storage = Instance.new("Model")
    for _, cloth in pairs(clothing) do
        if cloth:IsA("Clothing") then
            cloth.Parent = storage
        end
    end
    wait()
    local newShirt = script.Parent.Parent.Parent.Clothes.Shirt:clone()
    local newPants = script.Parent.Parent.Parent.Clothes.Pants:clone()
    newShirt.Parent = character
    newPants.Parent = character
    wait(5)
    newShirt:Destroy()
    newPants:Destroy()
    for _, cloth in pairs(storage:GetChildren()) do
        cloth.Parent = character
    end
end

script.Parent.ClickDetector.MouseClick:connect(onClicked)

Once again, this is fairly repetitive. You're reparenting clothes 4 times. Make a function to do that:

function moveClothes(from, to)
    for _, child in pairs(from:GetChildren()) do
        if child:IsA("Clothing") then
            child.Parent = to
        end
    end
end

function onClicked(player)
    local character = player.Character
    local clothing = character:GetChildren()
    local storage = Instance.new("Model")
    moveClothes(character, storage); -- Put original clothes safely away
    wait()
    moveClothes(script.Parent.Parent.Parent.Clothes:Clone(), character)
    -- Get a copy of the staged clothes, put them in the character
    wait(5)
    moveClothes(character, nil) -- Delete the show clothes
    moveClothes(storage, character) -- Put original clothes back
end

script.Parent.ClickDetector.MouseClick:connect(onClicked) 

-- Shorter (by 1/3)
-- Simpler (no loops, no ifs)
-- Clearer and more correct

Thanks. if its the tab that is the problem, I am sorry, but even if I did do the tabs it would still not make me wear them. Anyway thanks for answering my question and reporting it to the staff. User#5689 -1 — 9y

What is wrong with this script?

1 answer

Answer this question