Better organization?

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)
1 Like

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.

1 Like

100% was a test, and I just realized I put it twice, thanks!

1 Like

You should put it outside the loop too as it is just running the same piece of code again and again when it only needs to run once.

1 Like

In this single script, the rest of the code has to wait for the loop to complete, doesn’t it?

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)
1 Like

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!

4 Likes

Would you mind sharing your style guide?

1 Like

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.

2 Likes

Maybe have a popup after touched?

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.

1 Like

Don’t worry, just take your time!

1 Like

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.

1 Like