Button Code Review

DarkMenacing · March 13, 2022, 6:34pm

hi, i wanted to make a credit screen but first i needed a button so i made this script:

local player = game.Players.LocalPlayer

script.Parent.MouseButton1Click:Connect(function()
	script.Parent.Parent.Parent:FindFirstChild("Credits").Visible = true
	game:GetService("ReplicatedStorage"):FindFirstChild("Sounds"):FindFirstChild("ClickingSound"):Play()
	print(player.Name.." Opened Settings Gui")
	if script.Parent.Parent.Parent.Welcome.Visible == true then -- // If the Welcome Screen is visible, i will cancel the from opening the GUI
		script.Parent.Parent.Parent:FindFirstChild("ErrorWelcome").Visible = true
		script.Parent.Parent.Parent:FindFirstChild("Credits").Visible = false
		script.Parent.Parent.Parent:FindFirstChild("Data"):FindFirstChild("ErrorSound"):Play()
		wait(2)
		script.Parent.Parent.Parent:FindFirstChild("ErrorWelcome").Visible = false
	end
	if script.Parent.Parent.Parent:FindFirstChild("Settings").Visible == true then --// Check If any GUI Is Open
		script.Parent.Parent.Parent:FindFirstChild("Settings").Visible = false
	elseif script.Parent.Parent.Parent:FindFirstChild("Feedback").Visible == true then
		script.Parent.Parent.Parent:FindFirstChild("Feedback").Visible = false
	elseif script.Parent.Parent.Parent:FindFirstChild("GamepassShop").Visible == true then
		script.Parent.Parent.Parent:FindFirstChild("GamepassShop").Visible = false
	end
end)

the only thing i need is get all the 3 guis without using so much lines and check if one of them is visible to invisble.

a19_9 · March 14, 2022, 9:42pm

i think adding a local parent = script.Parent.Parent.Parent line will benefit you alot

TyePHX · March 14, 2022, 9:50pm

Can’t you put them all under one or two screengui and just set it as enabled/visible

Use task.wait to its more accurate

Brickman808 · March 14, 2022, 10:44pm

You can use variables to prevent repetition here.

No need to use wait here.

Might as well use . or WaitForChild here.

DarkMenacing · March 15, 2022, 1:45pm

The reason why i used wait(2) is because to show a message to the player’s client. if no wait(2) then the will show in a milisecond and fastly becomes not visible

DarkMenacing · March 15, 2022, 2:00pm

This the first result of the script:

local player = game.Players.LocalPlayer
local parent = script.Parent.Parent.Parent
local Data = game:GetService("ReplicatedStorage").Sounds
script.Parent.MouseButton1Click:Connect(function()
	parent:WaitForChild("Credits").Visible = true
	Data:WaitForChild("ClickingSound"):Play()
	print(player.Name.." Opened Settings Gui")
	if parent.Welcome.Visible == true then -- // If the Welcome Screen is visible, i will cancel the from opening the GUI
		parent:WaitForChild("ErrorWelcome").Visible = true
		parent:WaitForChild("Credits").Visible = false
		Data:WaitForChild("ErrorSound"):Play()
		task.wait(2)
		parent:WaitForChild("ErrorWelcome").Visible = false
	end
	if parent:WaitForChild("Settings").Visible == true then --// Check If any GUI Is Open
		parent:WaitForChild("Settings").Visible = false
	elseif parent:WaitForChild("Feedback").Visible == true then
		parent:WaitForChild("Feedback").Visible = false
	elseif parent:WaitForChild("GamepassShop").Visible == true then
		parent:WaitForChild("GamepassShop").Visible = false
	end
end)

me_isidoit · March 15, 2022, 2:21pm

If you’re 100% sure that all the UI loaded before the player gets a chance to click this button (which I think is the case here), then I wouldn’t use WaitForChild at all because it slows your code down. If you ever have to use WaitForChild, then I recommend setting it to a variable instead of calling it multiple times on the same instance.
I recommend using more variables too.

Instead of this unnecessary pile of if statements, you can use the not keyword to invert the boolean to clear and simplify your code, like so:

parent.Settings.Visible = not parent.Settings.Visible;
parent.Feedback.Visible = not parent.Feedback.Visible;
parent.GamepassShop.Visible = not parent.GamepassShop.Visible;

You can also remove the print line if you don’t need it.

Other than that, it looks good.

DarkMenacing · March 15, 2022, 2:46pm

I made this shorter script with a module:

local player = game.Players.LocalPlayer
local parent = script.Parent.Parent.Parent
local Module = require(script.Parent.ModuleScript)
local Data = game:GetService("ReplicatedStorage").Sounds
script.Parent.MouseButton1Click:Connect(function()
	parent:WaitForChild("Credits").Visible = true
	Data:WaitForChild("ClickingSound"):Play()
	if parent.Welcome.Visible == true then Module:ShowWelcomeError(parent.ErrorWelcome) end -- // If the Welcome Screen is visible, i will cancel the from opening the GUI
	if parent.Settings.Visible == true then parent.Settings.Visible = false end
	if parent.Feedback.Visible == true then parent.Feedback.Visible = false end
	if parent.GamepassShop.Visible == true then parent.GamepassShop.Visible = false end
end)

and what in the module:

local module = {}

function module:ShowWelcomeError(gui)	
	gui:WaitForChild("ErrorWelcome").Visible = true
	gui:WaitForChild("Credits").Visible = false
	game:GetService("ReplicatedStorage").Sounds:WaitForChild("ErrorSound"):Play()
	task.wait(2)
	gui.Visible = false
end

return module

me_isidoit · March 15, 2022, 3:03pm

Oh, it looks like I misinterpreted the if statements. But still, you can remove them. It’s unnecessary to check if the UI is visible in this case. You can just set Visible to false without using the if statements.

DarkMenacing · March 15, 2022, 3:31pm

im saying that if the player joined and he have the welcome screen on and he trys to open a gui it should show a error message.

DarkMenacing · March 15, 2022, 3:39pm

and if he doenst have the welcome screen on and have settings screen on and he trys open another it should allow he do but he have to close the settings screen.

Inf_idel · April 1, 2022, 3:44pm

Please note; This is all untested code and I have no clue if it actually works. Just a simple way of how I feel I would go about doing this things. One thing you can do to use less lines is just moving it all onto one line such as,

if mainFrame:FindFirstChild(i).Visible == true then return true; else return false; end

local plr = game.Players.LocalPlayer
local mainFrame = script.Parent.Parent.Parent

--// You can move these functions to a module

local function checkVisible(i)
   if mainFrame:FindFirstChild(i).Visible == true then
      return true;
  else
      return false;
   end
end

--[[local function changeAllState()
   if mainFrame:FindFirstChild('GamepassMenu').Visible == true then mainFrame.GamepassMenu.Visible = false elseif mainFrame:FindFirstChild('Settings').Visible == true then mainFrame.Settings.Visible = false elseif mainFrame:FindFirstChild('Feedback').Visible == true then mainFrame.Feedback.Visible = false end
end]]

local function changeState(i)
   if mainFrame:FindFirstChild(i).Visible == true then
      mainFrame:FindFirstChild(i).Visible = false
   else
      mainFrame:FindFirstChild(i).Visible = true
   end
end

--// You can obviously further organize the code below but this is just a rough example of how I would do it.

script.Parent.MouseButton1Click:Connect(function()
   changeState('Credits')
   game:GetService('ReplicatedStorage'):FindFirstChild('Sounds'):FindFirstChild('ClickingSound'):Play()
   local welcomeCheck = checkVisible('Welcome')
   if welcomeCheck then
      changeState('ErrorWelcome'), changeState('Credits'), mainFrame:FindFirstChild('Data'):FindFirstChild('ErrorSound'):Play()
      task.wait(2)
      changeState('ErrorWelcome')
   end
   if mainFrame:FindFirstChild('GamepassMenu').Visible == true then mainFrame.GamepassMenu.Visible = false elseif mainFrame:FindFirstChild('Settings').Visible == true then mainFrame.Settings.Visible = false elseif mainFrame:FindFirstChild('Feedback').Visible == true then mainFrame.Feedback.Visible = false end
end)

how i would display my code in an actual commission

local plr = game.Players.LocalPlayer
local mainFrame = script.Parent.Parent.Parent

local function checkVisible(i) if mainFrame:FindFirstChild(i).Visible == true then return true; else return false; end end

local function changeState(i) if mainFrame:FindFirstChild(i).Visible == true then mainFrame:FindFirstChild(i).Visible = false else mainFrame:FindFirstChild(i).Visible = true end end

script.Parent.MouseButton1Click:Connect(function()
   changeState('Credits')
   game:GetService('ReplicatedStorage'):FindFirstChild('Sounds'):FindFirstChild('ClickingSound'):Play()
   local welcomeCheck = checkVisible('Welcome')
   if welcomeCheck then
      changeState('ErrorWelcome'), changeState('Credits'), mainFrame:FindFirstChild('Data'):FindFirstChild('ErrorSound'):Play()
      task.wait(2)
      changeState('ErrorWelcome')
   end
   if mainFrame:FindFirstChild('GamepassMenu').Visible == true then mainFrame.GamepassMenu.Visible = false elseif mainFrame:FindFirstChild('Settings').Visible == true then mainFrame.Settings.Visible = false elseif mainFrame:FindFirstChild('Feedback').Visible == true then mainFrame.Feedback.Visible = false end
end)

DarkMenacing · April 1, 2022, 10:39pm

Thank You, i will test it and if it works i will set it as solution!
(Also, the data folder isnt located in the mainframe )

Inf_idel · April 1, 2022, 10:49pm

I wasn’t meaning to completely recreate the script I was just trying to show you how I would do it, you would of course need to set it up correctly for yourself and how you have it all setup. I wrote it during school on a small computer screen.

DarkMenacing · April 1, 2022, 10:50pm

yeah i know how set it up by my self