Basic Style Improvements
Tab your code correctly
Use generic for loops (You're reusing i which is a little scary)
function onClicked()
local players = game.Players:GetChildren()
for _, player in pairs(players) do
if (player.Character.Torso.Position - script.Parent.Position).magnitude < dist then
local name = player.Name
local clothing = player:GetChildren()
for _, cloth in pairs(clothing) do
if cloth: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
Address Issues
Now, some obvious problems: clothing doesn't make sense. A player won't have any useful children -- its clothing are in its Character.
You define f to be the character, but in a completely roundabout way. You also might as well use workspace[name] since you don't do error checking on f.
player.Character could be nil, too, so we have to check for that.
Also, you don't ever use the variable cloth, which is usually a big warning flag. (You instead attempt to indirectly refer to it using s and p)
s and p could be nil, but you do stuff to them anyway.
You use a lot of script.Parents, but never script on its own, so why not make that a variable?
It seems like instead you should move all the cloth objects to PlayerStorage, move the new objects in (not in the loop) and then move them back:
local button = script.Parent;
function onClicked()
local players = game.Players:GetChildren()
for _, player in pairs(players) do
local character = player.Character
if character and (character.Torso.Position - button.Position).magnitude < dist then
local clothing = character:GetChildren()
for _, cloth in pairs(clothing) do
if cloth:IsA("Clothing") then
cloth.Parent = button.Parent.PlayerStorage
end
end
local shirt = button.Parent.Parent.Clothes.Shirt:clone()
local pants = button.Parent.Parent.Clothes.Pants:clone()
shirt.Parent = character
pants.Parent = character
wait(5)
shirt:Destroy()
pants:Destroy()
for _, cloth in pairs(button.Parent.PlayerStorage:GetChildren()) do
if cloth:IsA("Clothing") then
cloth.Parent = character
end
end
end
end
end
button.ClickDetector.MouseClick:connect(onClicked)
Actually though we probably don't want to store things in PlayerStorage -- other players might use that at the same time. We can just make a temporary model:
storage = Instance.new("Model")
....
cloth.Parent = storage
....
Use Functions
We are reusing code: we use two four loops to moves Clothing, and also we also move Clothing in two other places. Why don't we make a moveClothes function?
If we're doing something 4 times, the answer is probably to use a function.
function moveClothes(from, to)
for _, child in pairs(from:GetChildren()) do
if child:IsA("Clothing") then
child.Parent = to
end
end
end
function onClicked()
local players = game.Players:GetChildren()
for _, player in pairs(players) do
local character = player.Character
if character and (character.Torso.Position - button.Position).magnitude < dist then
local storage = Instance.new("Model")
moveClothes(character, storage);
moveClothes(button.Parent.Parent.Clothes:Clone(), character);
wait(5)
moveClothes(character, nil);
moveClothes(storage, character);
end
end
end
Now it reads very simply:
- Move the Character's clothes into storage
- Move a copy of the model clothes into the Character
- Wait a few seconds
- Get rid of the character's clothes (which are a copy of the model)
- Move the clothes back from storage onto the character
1 comment
Thank you soo much bro. But I want to apologize since this must have been a lot of work for you to explain and type all those for me.