Is this script easy to understand?

Help and Feedback Scripting Support
#1

I am writing a script that changes through frames for a Character Customizer GUI and I wanted to know if it was clear and as simple but efficient as possible.

I am working with a couple of other people (not scripters) on a game and hope that if they need to change something without me they will understand and be able to.

If you could give me feedback and suggestions on what to add/change that would be helpful (I am still in the learning process for scripting).

If anyone wants to take this script for their own use feel free but please do not upload it to the Toolbox.

The script:

local Frame = script.Parent
local CurrentFrame = nil
local Buttons = Frame.Parent.ButtonBar

local HatFrame = Frame.HatPicker
local ClothesFrame = Frame.ClothingPicker
local SkinFrame = Frame.SkinColor

local HatButton = Buttons.Hat
local HairButton = Buttons.Hair
local ClothesButton = Buttons.Clothing
local SkinButton = Buttons.Skin

-- Hats
HatButton.MouseButton1Click:Connect(function()
	local	CurrentFrame = HatFrame
	local OtherFrames = ClothesFrame and SkinFrame
	if CurrentFrame.Visible == false then
		CurrentFrame.Visible = true
		if OtherFrames.Visible == true then
			OtherFrames.Visible = false
		end	
	end	
end)


-- Clothing
ClothesButton.MouseButton1Click:Connect(function()
	local	CurrentFrame = ClothesFrame
	local	OtherFrames = SkinFrame and HatFrame
	if CurrentFrame.Visible == false then
		CurrentFrame.Visible = true
		if OtherFrames.Visible == true then
			OtherFrames.Visible = false
		end	
	end	
end)


-- Skin
SkinButton.MouseButton1Click:Connect(function()
	local	CurrentFrame = SkinFrame
	local	OtherFrames = ClothesFrame and HatFrame
	if CurrentFrame.Visible == false then
		CurrentFrame.Visible = true
		if OtherFrames.Visible == true then
			OtherFrames.Visible = false
		end	
	end	
end)
1 Like
#2

Instead of doing this you can simply do


   CurrentFrame.Visible = not CurrentFrame.Visible

So if the visible is true, it will set to not true(false), and if it is false, it will set it to not false(true).

Yes, your script is understandable

1 Like
#3

This is exactly like just writing OtherFrames.Visible = false. The only case where the if-clause isn’t entered is when Visible is already false. So if Visible is true then it’s set to false, and if it’s false then it just stays false.

This approach doesn’t work. and is a boolean operator, meaning it evaluates to a single value. In this case it’ll always just be the first thing because it’s not false or nil, so ClothesFrame. Check out Programming in Lua : 3.3. It looks like you want to keep around a collection of several values (2 object references in this case). Try this instead to create a list of objects and do something to each of them:

HatButton.MouseButton1Click:Connect(function()
	local CurrentFrame = HatFrame
        CurrentFrame.Visible = true --No need to check if it was false before, because setting it to true "again" doesn't change anything
        
	local OtherFrames = {ClothesFrame, SkinFrame}
    for _, otherFrame in ipairs(OtherFrames) do
        otherFrame.Visible = false
    end
end)

Frame is a pretty bad variable name. If the object is also just called Frame then that’s a pretty bad name for it too. Change it to something that explains what it does/is for, not just what class of object it is. E.g. Container, Main, Background, whatever fits best with your understanding of what it’s for.

It’s a lot better than just Frame, but HatMenu would be even better because that’s what it actually is. Or maybe HatTab if your layout looks like a tabbed layout.

Again it’s okay, but HatMenuButton or even HatMenuToggleButton would be better because it explains what it’s for - there’s most likely going to be lots of buttons inside the hat menu, and a more verbose name like that is necessary to differentiate it from all the other ones.

If you change the menus to be called SomethingMenu, this one should be changed to match, e.g. CurrentMenu. You could be a bit more precise in what the “current thing” currently “is”, e.g. OpenedMenu (OpenMenu is bad because Open is also a verb, making it sound like a function).

Other than that, yeah it’s pretty good. You could get a lot more abstract and make your script “simpler” by just having a single function for setting up a menu like this, but less code is not always more readable and in this case I think you’re good to go.

If you really want your designers to have the best change to modify the GUI without breaking the script, you could have a better approach to “finding” the different objects. Right now you’re 100% dependent on a very strict hiearchy, e.g. HatFrame must be inside Frame which must be inside script.Parent. If the designers e.g. want to put each button inside another Frame so they can add a UIPadding or something, your script would break. There’s also no easy way to add or remove menus without having to modify the script. It can be set up to be a bit more robust, but it adds complexity and TBH I think there’s a limit to how robust you should even try to make it, because it’s very likely that the design will change in a way that’s not supported and then there’s just 2x as much code to fix. My suggestion is to try and finalize the design more or less and then code it to actually work. But let me know if you want to see how a more “automatic”/change- resistant approach could work.

1 Like