Scripting HelpersARCHIVE
A little piece of Roblox history. Preserved for everyone, one question at a time.Read about the archive →
COMMUNITY QUESTION

What is wrong with this script?

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) 


1 comment

1

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.

1 answer

  1. Tab your code correctly.

  2. Use generic for loops: i is even reused twice! But it's not used anywhere except to index the thing your'e looping over.

  3. MouseClick tells you playerWhoClicked. Don't use that loop. It's unnecessary.

  4. 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

1 comment

0

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.

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