This falling brick code is ok?

Help and Feedback Code Review
#1

I have made a basic spleef system, but I don’t know if the code I made (which is for each brick, that if touch it, the brick gets unanchored [basically, a falling brick]) is correctly made, I want to know if my code is ok, and if there is a way to optimize it, or if the code is already optimized, here is the code:

local Block = script.Parent
local Touched = false

Block.Touched:Connect(function(obj)
    if not Touched then
        Touched = true
        script.Parent.Color = Color3.fromRBG(255,0,0)
        script.Parent.Anchored = false
        wait(1)
        Block:Destroy()
    end
end)

It’s a basic code, but who knows if program basic stuff is a good way to advance your development. Thanks for reading

1 Like
#5

A good way to check code optimization is via Studio’s Script Performance option. The lower, the better.

Also, the Touched and if not Touched seems to serve no purpose.

2 Likes
#6

I thought doing the Not touched would optimize the code, but thanks for your help!

#7

Hello!
Right now, your script is not functional. You should replace the command fromRBG() with fromRGB(). :wink:

Something that could be helpful is replacing script.Parent with Block, since it is a recurrent reference and it is already defined by a variable.

Yielding functions should be avoided when they are not necessary. Instead of delaying Destroy() you could simply add the object to Debris.

This script has a huge problem when put in context. Once a part is touched, it will become unanchored - meanwhile, surrounding parts are listening to the touched event. This means that a player would have to step on a single spleef block to sink all the other bricks (as long as the distance between them is minimal). Checking if the object that hit the part is a humanoid won’t make this a problem.

Finally, I think you did a good job with the debounce. We wouldn’t want to set a part’s color potentially tens or even hundreds of times in a second! However, in cases like this another option would be disconnecting the event, since you don’t want it to fire anymore after you touched it. Again, you could use Touched:Wait() to fire the event once.

This is how I would have scripted the part: 
local Block = script.Parent
local Debris = game:GetService("Debris")
local obj = Block.Touched:Wait()

if obj.Parent:FindFirstChild("Humanoid") then
	Block.Color = Color3.fromRGB(255,0,0)
	Block.Anchored = false
	Debris:AddItem(Block, 1)
end
Another option: 
local Block = script.Parent
local Debris = game:GetService("Debris")

Connection = Block.Touched:Connect(function(obj)
	if obj.Parent:FindFirstChild("Humanoid") then
		Block.Color = Color3.fromRGB(255,0,0)
		Block.Anchored = false
		Debris:AddItem(Block, 1)
		Connection:Disconnect()
	end
end)

Remember that there are always many methods when programming, but there is not necessarily a best way to work.

These articles can be helpful:

4 Likes
#9

No, not really. Using the profiler is much better.

1 Like