Any opinions to improve my code?

ByGermanKnight · February 25, 2021, 7:12pm


local TweenService = game:GetService("TweenService")
local MainOpen = script.Parent.MainOpen
local FastChat = script.Parent
local Out = true

local Open = false
local MainButtons = script.Parent.MainButtons

local RotateTween1 = TweenService:Create(script.Parent.MainOpen, TweenInfo.new(1, Enum.EasingStyle.Quad, Enum.EasingDirection.Out), {Rotation = 180})
local RotateTween2 = TweenService:Create(script.Parent.MainOpen, TweenInfo.new(1, Enum.EasingStyle.Quad, Enum.EasingDirection.Out), {Rotation = 0})

MainOpen.MouseButton1Click:Connect(function()
	if Out == true then
		RotateTween1:Play()
		
		FastChat.MainButtons:TweenPosition(UDim2.new(0.911, 0,0.304, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
		FastChat.MainOpen:TweenPosition(UDim2.new(0.878, 0,0.465, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
		Out = false
	else
		RotateTween2:Play()
		
		script.Parent.Raenge:TweenPosition(UDim2.new(1, 0,0.304, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
		script.Parent.RaengeClose:TweenPosition(UDim2.new(1, 0,0.303, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
		
		FastChat.MainButtons:TweenPosition(UDim2.new(1, 0,0.304, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
		FastChat.MainOpen:TweenPosition(UDim2.new(0.963, 0,0.465, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
		Out = true
	end
end)


for i, v in pairs(MainButtons:GetChildren()) do
	if v:FindFirstChild("ImageButton") then
		v.ImageButton.MouseButton1Click:Connect(function(clicker)
			if Open == true then
				FastChat.Raenge:TweenPosition(UDim2.new(1, 0,0.304, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
				Open = false
			else
				FastChat.Raenge:TweenPosition(UDim2.new(0.911, 0,0.304, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
				FastChat.RaengeClose:TweenPosition(UDim2.new(0.911, 0,0.303, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
				Open = true
			end
		end)
	end
end

FastChat.RaengeClose.MouseButton1Click:Connect(function()

	FastChat.Raenge:TweenPosition(UDim2.new(1, 0,0.304, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
	FastChat.RaengeClose:TweenPosition(UDim2.new(1, 0,0.303, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Quad, 1, true)
	Open = false

end)

colbert2677 · February 25, 2021, 10:58pm

What are you looking for improvements on, specifically? What part of code are you unsatisfied with and how are you looking to improve it? There’s not enough information on what kind of improvements you’re looking for, this is just a giant chunk of code slapped into a thread. Please check out the category guidelines, it has some information on how best to structure a thread if you want to get your code reviewed.

BaseplateB0T · February 26, 2021, 4:27am

yeah code either works or it doesn’t are you looking for optimization changes

Spacerator · February 26, 2021, 9:09am

Your code looks generally fine, with a some remarks:

Using UDim2.new(0.911, 0, 0.304, 0) is a bit strange to me. Are you sure you can’t do pixel offsets there or are you firm on going with scale? (also you have spacing issues after the second argument in all UDim2.new()s)
FastChat should be declared before MainOpen and MainOpen should be: MainOpen = FastChat.Parent
You declare FastChat but use script.Parent on multiple occasions. Why? You already have a variable for that.
Same thing with multiple other variables:

-- You have already declared FastChat, MainOpen and MainButtons, yet you do this
MainOpen.MouseButton1Click:Connect(function()
   --code omitted for clarity

   FastChat.MainButtons:TweenPosition()--[...]
end)

Why not just use MainButtons instead of indexing FastChat again?

The proper spelling is “Range”, not “Raenge”.
On that note, separate Range into a variable instead of doing FastChat.Range or script.Parent.Range
Don’t declare i in the for loop if you’re not going to use it. It should look like this:

for _, v in pairs(MainButtons:GetChildren()) do

Don’t do Out == true. Just write Out! If you want to check if Out is false, write not Out.
Your overall formatting is weird. On the RangeClose.MouseButton1Click function, you have an empty line at the top and the bottom, which is not present in any other block of code from the script you shared.

angrybino · February 26, 2021, 9:55am

Don’t declare i in the for loop if you’re not going to use it. It should look like this:

That is just a naming convention people use to mark unneeded variables, not something to take in count of when refactoring code except for readability.

Don’t do Out == true . Just write Out ! If you want to check if Out is false, write not Out .

Doesn’t matter, this also doesn’t improve OP’s code.

@OP:

Use ipairs instead of pairs, ipairs should always be used on arrays since GetChildren returns an array.

That’s all for a small amount of code.

Spacerator · February 26, 2021, 9:57am

Luau optimises code to the point where that is not a problem with optimisation but just readability. Micro optimisations were not my point, readability was.

Out instead of Out == true does improve OP’s code, because the time it takes to run is just as significant as how easily it can be read.

angrybino · February 26, 2021, 10:01am

No it doesn’t improve OP’s code, Out == true is more readable than just Out. Also, the readability of OP’s code is already good, there is no need to make pointless points.

PS: That wasn’t even a micro-optimization to begin with, whatever you said makes 0 sense.

Spacerator · February 26, 2021, 10:06am

I disagree severely.

if Out then

… is always going to be more readable, because it is closer to an English sentence. Comparing booleans to true and false is bad practice in every language (even compiled ones!) because it’s less readable and non-standard.

The readability of OP’s code is not good. They declare variables that aren’t used and any future maintainer could possibly spend hours trying to figure out why does changing that variable do nothing???, especially if the script is longer.

In response to your edit: yes, you said it was!

That is just a naming convention people use to mark unneeded variables, not something to take in count of when refactoring code except for readability.

What is it for, if it’s not for readability?

angrybino · February 26, 2021, 10:08am

… is always going to be more readable, because it is closer to an English sentence. Comparing booleans to true and false is bad practice in every language (even compiled ones!) because it’s less readable and non-standard.

No it isn’t bad practice, that is completely false. We’re talking about readability in scripts, not in English. If Out == true then is more readable than if Out then. What you’re saying is subjective.

The readability of OP’s code is not good. They declare variables that aren’t used and any future maintainer could possibly spend hours trying to figure out why does changing that variable do nothing??? , especially if the script is longer.

I’m talking about how they named their variables, not on what that variable does.

What is it for, if it’s not for readability?

Making negligible readability improvements doesn’t improve OP’s code.

Spacerator · February 26, 2021, 10:24am

Making negligible readability improvements doesn’t improve OP’s code.

Readability improvements should never be called “negligible”. They’re improvements, and readability is standardized.

No it isn’t bad practice, that is completely false.

Oh boy…
Here’s a link to an article by the luarocks maintainers, which clearly states that this is bad for readability.
LuaRocks is probably the best-known package manager for Lua. I’d say they know a thing or two.
Another Lua answer, this one explicitly mentions “standard boolean techniques”
We could argue that everything about the syntax that is non-standard is bad for readability. Standards have a reason to exist.

And since I mentioned other languages:
Java
Python
C# - same concept, example with ! (which is not for us)
I could go on.

Edit: updated luarocks link

angrybino · February 26, 2021, 10:27am

Readability improvements should never be called “negligible”. They’re improvements, and readability is standardized.

That is again, somewhat of a subjective statement. Just because you say it, doesn’t mean it’s right. That is a very small readability improvement, the readability of the code is fine enough.

I would rather compare explicitly by == true operator instead of just value.

Spacerator · February 26, 2021, 10:30am

What is subjective about it? That code should be standardized? And that people should follow these standards?

ByGermanKnight · February 27, 2021, 7:04pm

Thank you for the help. And “Raenge” is German and means Ranks.