How i make this script "clean"?

Help and Feedback Code Review
#1

So I need to make this code “cleaner”, do you have any ideas or examples, thanks!

local frame = script.Parent.Parent.GameFrame
local b1 = script.Parent.Parent.GameFrame.Button1
local b2 = script.Parent.Parent.GameFrame.Button2
local b3 = script.Parent.Parent.GameFrame.Button3

local button = script.Parent

if button.MouseButton1Click:Connect(function()
		frame.Visible = true
	end)
then

end
repeat
	wait(1)
until
b1.MouseButton1Click:Connect(function()
	b1.Visible = false
	print("prints")
end)
if b1.Visible == false then
else
	repeat
		wait(1)
	until
	b1.Visible == false
end
repeat
	wait(1)
until
b2.MouseButton1Click:Connect(function()
	b2.Visible = false
		print("prints")
	end)
#2

I think you should refrain from using too many loops, those can slow down the game.

1 Like
#3

But how I would do that is the question sir

#4

I have questions about this script you wrote, a lot of them actually.
Let’s start with line 8-13:

if button.MouseButton1Click:Connect(function()
		frame.Visible = true
	end)
then

What is the point of this if statement exactly? This will always evaluate to true because :Connect() function always returns a RBXScriptConnection which in Luau is a datatype that is not false or nil.

Same thing goes for line 14-20. Again :Connect() function always returns a RBXScriptConnection which evaluates to true so the script only waits for one second because of the way repeat-until loops work.

Same thing again for line 28-34.

You don’t need if statements and loops for these things I mentioned.

2 Likes
#5 
local frame = script.Parent.Parent.GameFrame
local b1 = script.Parent.Parent.GameFrame.Button1
local b2 = script.Parent.Parent.GameFrame.Button2
local b3 = script.Parent.Parent.GameFrame.Button3

local button = script.Parent

button.MouseButton1Click:Connect(function()
	frame.Visible = true
end)

b1.MouseButton1Click:Connect(function()
	b1.Visible = false
	print("prints")
end)

b2.MouseButton1Click:Connect(function()
	b2.Visible = false
	print("prints")
end)

Here is the code cleaned. There was a lot of unneeded lines. For example, repeat loops.

2 Likes
#6

Adding onto Nightmare’s post, you can clean up the code a little more… Since

script.Parent.Parent.Gameframe

Is already defined, you can use that to get your other variables without repeating it. I have made a cleaner example below.

local Frame = script.Parent.Parent.GameFrame
local B1, B2, B3 = Frame.Button1, Frame.Button2, Frame.Button3

local Button = script.Parent

Button.MouseButton1Click:Connect(function()
	Frame.Visible = true
end)

B1.MouseButton1Click:Connect(function()
	B1.Visible = false
	print("prints")
end)

B2.MouseButton1Click:Connect(function()
	B2.Visible = false
	print("prints")
end)
1 Like
#7

Make sure you use WaitForChild instead of directly indexing for it as well. There’s no certainty that the script is ran after everything is loaded.

local frame = script.Parent.Parent:WaitForChild("GameFrame")
local b1 = frame:WaitForChild("Button1")
local b2 = frame:WaitForChild("Button2")
local b3 = frame:WaitForChild("Button3")
2 Likes
#8

This is unnecessary and not true. The local script is a descendant of the frame which means the frame and its other children will load before it runs.

#9

What are you on about? Everything necessary for the client has to be replicated to the client from the server and not all instances are guaranteed to be loaded in time when a local script runs.

#10

So im making like 3 buttons and first u cant click the 2 and 3 but 1 you can and if you click1 you can cick 2 then 3, I was thinking if this script need to be cleaner I think now maybe NO so ye!

#11

First of all you don’t need loops at all, you can make use of .Changed event or :GetPropertyChangedSignal("Value") on a IntValue to detect the current value.

#12

Something like this.

local IntValue = script:WaitForChild("IntValue")

local player
local mouse

local currentValue

IntValue.Changed:Connect(function(value)
    currentValue = value
end)

mouse.Button1Down:Connect(function()
   if currentValue >= 3 then
      -- you have unlocked all
   else
     -- you only have unlocked 2 or 1
   end
end)

keep in mind that this is completely exploitable on the client, you can easily secure it via RemoteEvent.

1 Like
#13

This line, the first line they suggested, was fine. It yields and waits for the main frame.

local frame = script.Parent.Parent:WaitForChild("GameFrame")

However this additional suggestion is useless.

local b1 = frame:WaitForChild("Button1")
local b2 = frame:WaitForChild("Button2")
local b3 = frame:WaitForChild("Button3")

This line comes after the initial yield, so these literally won’t error if frame:WaitForChild("Button3") was switched to frame.Button3.

While this is true in some instances, such as a server script looking into ClientGui stuff, the situation here doesn’t apply.

#15

While this is true in some instances, such as a server script looking into ClientGui stuff, the situation here doesn’t apply.

Did I ever say this about server? No, please read my post properly.

Everything necessary for the client

This line still is fine if frame has a lot of descendants because if it has loaded, it still for some reason isn’t guaranteed that literally all it’s descendants are going to be loaded.

local b1 = frame:WaitForChild("Button1")
local b2 = frame:WaitForChild("Button2")
local b3 = frame:WaitForChild("Button3")
#16

I was just pointing out a fact, not sure why you’re so defensive over something so small.

Is this even proven?

Going back to the original script provided:

local frame = script.Parent.Parent.GameFrame

This line suggests that the script is a long descendant of the main frame. By the time the script even has a chance to run, the other buttons would have already loaded.

Adding those extra WaitForChild()s is a waste of a yield.

#17

This line suggests that the script is a long descendant of the main frame. By the time the script even has a chance to run, the other buttons would have already loaded.

Nope, not always true.

I was just pointing out a fact, not sure why you’re so defensive over something so small.

Because it’s irrelevant at what I was talking about.

#18

I’ve never heard of that after years of scripting. And even if you are right, it’s most likely only in extreme cases where there are tons of frame elements in the GUI.

#19

It’s not in extreme cases actually, it does persist normally and usually for me, it always happens.

#20

If it already exists, it won’t waste a yield. There is no reason not to do this. And there always a chance that it hasn’t loaded in. I learned it the hard way when I had to use pick nested UI elements. Just because the parent is loaded in doesn’t mean the descendants are.