This does everything that I need, I just want to become a more organized scripter and the most efficient as possible.
I can’t think of improvements.
I just want to see how I could’ve done this more efficiently, maybe quicker, and overall shorter and easier to read.
Provide an overview of:
What does the code do and what are you not satisfied with?
What potential improvements have you considered?
How (specifically) do you want to improve the code?
local touched = false
local bv = game.ReplicatedStorage.ParachuteVelocity
script.Parent.Touched:Connect(function(player)
if not touched then
touched = true
local chance = math.random(0, 100)
if chance <= 100 then
local bcCopy = bv:Clone()
local bccCopy = bv:Clone()
bccCopy.Parent = player
bcCopy.Parent = player.Parent.Chest
local children = player.Parent.Chest:GetChildren()
for i, child in ipairs(children) do
if child:IsA("UnionOperation") or child:IsA("Part") then
player.Parent.Chest.Middle.Transparency = 1
child.Transparency = 0
player.Parent.Chest.Middle.Transparency = 1
end
end
wait(10)
player.ParachuteVelocity:Destroy()
player.Parent.Chest:Destroy()
touched = false
end
end
end)
2 questions, chance has a 100% chance to be less than or equal to 100. Is it just for testing or? Also, you set player.Parent.Chest.Middle.Transparency = 1 twice for every child that Chest has.
Yep, bit it should pretty much happen instantly. Either way, running player.Parent.Chest.Middle.Transparency = 1 over and over again isn’t gonna do anything apart from make it a tiny bit slower, and harder to read. If you put it outside the loop then it would only run once. Example:
local bv = game.ReplicatedStorage.ParachuteVelocity
script.Parent.Touched:Connect(function(player)
if not touched then
touched = true
local chance = math.random(0, 100)
if chance <= 100 then
local bcCopy = bv:Clone()
local bccCopy = bv:Clone()
bccCopy.Parent = player
bcCopy.Parent = player.Parent.Chest
local children = player.Parent.Chest:GetChildren()
player.Parent.Chest.Middle.Transparency = 1
for i, child in ipairs(children) do
if child:IsA("UnionOperation") or child:IsA("Part") then
child.Transparency = 0
end
end
wait(10)
player.ParachuteVelocity:Destroy()
player.Parent.Chest:Destroy()
touched = false
end
end
end)
Hello! I gave your code a quick read and, over some coffee, rewrote it with some commentary. Keep in mind this is not a prescriptive “solution”, just pitfalls I see that could occur later down the line.
This code is using my own, internal style guide. It is very subjective.
-- Call me old fashioned, but I believe any service or import should be declared
-- at the top, even if it's only used once.
local ReplicatedStorage = game:GetService("ReplicatedStorage");
-- I would recommend avoiding magic numbers. Any number that's not -1, 0, or 1
-- I would assign to a "constant" variable, within reason.
local CHANCE = 1;
-- Perhaps a more descriptive name than "bv"? Also, I'd use "WaitForChild" since
-- ReplicatedStorage doesn't guarantee everything is replicated instantly.
local parachuteVelocity = ReplicatedStorage:WaitForChild("ParachuteVelocity");
-- It's advised you declare variables to be local both for better performance
-- and to not pollute the global namespace. (Good job!)
local touched = false;
-- I believe writing listeners and connecting them later in the script is more
-- readable and maintainable than inlining them (in the context of Lua).
-- Also I believe the parameter will not be the player object, but the character
-- model of the player? Should we check "character" is of a given type?
local function onTouched(character)
-- I believe terminating the function early as opposed to wrapping the body
-- of the function in one large if statement is more readable. It prevents
-- indentation hell.
if not touched then
touched = true;
else
return;
end
-- In my opinion, values representing chance seem more natural as decimals
-- than integers.
local randomValue = math.random();
-- I believe indenting here, however, increases readability.
if randomValue <= CHANCE then
local chest = character.Parent.Chest;
chest.Middle.Transparency = 1;
local parachuteVelocityPlayer = parachuteVelocity:Clone();
parachuteVelocityPlayer.Parent = character;
local parachuteVelocityPlayerChest = parachuteVelocity:Clone();
parachuteVelocityPlayerChest.Parent = chest;
-- I believe inlining "children" here is actually less readable.
for _, child in ipairs(chest:GetChildren()) do
-- We could just get every object that has a "Transparency"
-- property.
if child:IsA("BasePart") then
child.Transparency = 0;
end
end
wait(10);
-- We already have references to these objects, so we can call "Destroy"
-- directly as opposed to using a bunch of property access expressions
-- to get them again.
parachuteVelocityPlayer:Destroy();
parachuteVelocityPlayerChest:Destroy();
touched = false;
end
end
-- Yay! It's time to connect our event listener to the event.
script.Parent.Touched:Connect(onTouched);
If you have any questions about what something is or why I did something a certain way, I’d love to share my rationale!
I’m actually preparing a public release of it with rationales to release to the general public! I’ll have a functional link sometime this weekend, maybe Monday at the latest.
For the time being, if you have more specific questions before then, I can answer those.
It appears this is slowly yet surely getting pushed into the backlog of things to do; when it’s available, I’ll post it, although I’m unsure if that’s anytime soon.
One thing that I find cleans up the code a bit with Touched events is defining a debounce function which takes a function and returns a debounced version. This means you don’t have to write “if not touched then touched = true …” in every Touched handler.
There’s an example at the end of this wiki page under “Advanced Notation”.
Then you can put that function in a module and use it wherever.