How messy is my Script? And how can I improve it?

game.Players.PlayerAdded:Connect(function(plr)
	

	local tixBarGui = plr.PlayerGui:WaitForChild("TixBarGui")
	local tixBarBackground = tixBarGui.TixBarBackground
	local collectedTix = tixBarBackground.CollectedTix
	local valuesFolder = workspace.ValuesFolder
	local collectedTixValue = valuesFolder.CollectedTixValue
	local tixBar = tixBarBackground.TixBar
	local completionBarValue = valuesFolder.CompletionBarValue
	local playFindTixSFXEvent = game.ReplicatedStorage:WaitForChild("FindTixSFXEvent")
	local expressingEmoji1 = tixBarBackground.ExpressingEmoji1
	local expressingEmoji2 = tixBarBackground.ExpressingEmoji2
	local expressingEmojisValue = valuesFolder.ExpressingEmojisValue
	

local function checkCurrentEmoji() 
	if collectedTixValue.Value < 10 then

		expressingEmojisValue.Value = "10"
		expressingEmoji1.Text = expressingEmojisValue.Value
		expressingEmoji2.Text = expressingEmojisValue.Value

	elseif collectedTixValue.Value >= 10 and collectedTixValue.Value < 20 then
		
		expressingEmojisValue.Value = "20"
		expressingEmoji1.Text =expressingEmojisValue.Value
		expressingEmoji2.Text = expressingEmojisValue.Value

	elseif collectedTixValue.Value >= 20 and collectedTixValue.Value < 30 then

		expressingEmojisValue.Value = "30"
		expressingEmoji1.Text = expressingEmojisValue.Value
		expressingEmoji2.Text = expressingEmojisValue.Value

	elseif collectedTixValue.Value >= 30 and collectedTixValue.Value < 40 then

		expressingEmojisValue.Value = "40"
		expressingEmoji1.Text = expressingEmojisValue.Value
		expressingEmoji2.Text = expressingEmojisValue.Value

	end
end

-----------------------

	checkCurrentEmoji()
	
	collectedTix.Text = collectedTixValue.Value.."/100"
	
	tixBar:TweenSize(UDim2.new(completionBarValue.Value,0,1,0),
		Enum.EasingDirection.InOut,
		Enum.EasingStyle.Linear,
		0.5
	)
	
	for _, Tixes in pairs(workspace:GetDescendants()) do 

		if Tixes:IsA("Part") and Tixes.Name == "Tix" then 
			
			for _, ClickDetectors in pairs(Tixes:GetChildren()) do 
				
				if ClickDetectors:IsA("ClickDetector") and ClickDetectors.Name == "ClickDetector" then 
					ClickDetectors.MouseClick:Connect(function(playerWhoClicked) 
						
						local expressingEmoji1 = tixBarBackground.ExpressingEmoji1
						local expressingEmoji2 = tixBarBackground.ExpressingEmoji2
						local expressingEmojisValue = valuesFolder.ExpressingEmojisValue
						
						collectedTixValue.Value += 1
						
						collectedTix.Text = collectedTixValue.Value.."/100"
						
						completionBarValue.Value += 0.01
						
						tixBar:TweenSize(UDim2.new(completionBarValue.Value,0,1,0),

							Enum.EasingDirection.InOut,

							Enum.EasingStyle.Linear,

							0.75)
				
						
						playFindTixSFXEvent:FireClient(plr)
						
						checkCurrentEmoji()
						
						Tixes.CanCollide = false
						Tixes.Transparency = 1
						ClickDetectors.MaxActivationDistance = 0
						
						for i, TixesChildren in pairs(Tixes:GetChildren()) do
							
							if TixesChildren:IsA("ParticleEmitter") and TixesChildren.Name == "TixVFX" then
								TixesChildren.Transparency = NumberSequence.new(1)

							elseif TixesChildren:IsA("Decal") and TixesChildren.Name == "TixDecal" then
								
								TixesChildren.Transparency = 1
							end
						end
					end)
				end
			end
		end
	end
end)

cmon bruh

2 Likes

Well first of all, I am going to assume this is a server script.

I recommend that all UI updates/interactions that you do from now on are all done on the client. You don’t want to put stress on the server right? Just utilize remotes to update the UI.

All the variables are inside of the playeradded method. Considering that some of these variables are constants (things that do not change), it is unoptimized to keep updating the variable whenever the player joins.

The checkCurrentEmoji function can be condensed. For example,

expressingEmojisValue.Value = “10”
expressingEmoji1.Text = expressingEmojisValue.Value
expressingEmoji2.Text = expressingEmojisValue.Value

is repeated multiple times. Perhaps you can put this in another function for clarity.

In the for loop you check all descendants of workspace. Now that is something you should never do. Let’s say you have 20000 objects in workspace, you might actually break the server if you run this script. Instead, just put the Tix in a folder in workspace and loop through that.

Also, using pairs and ipairs is useless because Roblox does it for you now, so you can just remove them.

You do not have to use a for loop for the contents of Tix. Just do Tix.ClickDetector or Tix.ParticleEmitter. If you want to ensure that it is a certain object, do something like this:

if not Tix.ClickDetector:IsA(“ClickDetector”) then return end — exits the code if not true.

I can ensure you that if you follow all these steps, your code will be so much better. Let me know if you have trouble doing any of these, I might be on to help.

But this will make it exploitable, and when they’re exploited, exploiters can modify it and give themselves free progress (like setting collectedTixValue to 100, which will give them a free rebirth and 100 Tixes on the leaderstats, which is ridiculous.

So I made the code work on the Server instead.

And what’s the difference between writing the code on a Script and using FireServer() to make the code run on the server? Aren’t both the same?

Not sure what you mean by ‘variables that do not change’.

Should I write the variables outside of PlayerAdded?

Will do that :+1:

I already have a TixesFolder in the workspace, but I thought there’s no difference between looping through the folder and looping through workspace.Descendants.

I may also use CollectionService to implement this, instead of re-writing these if statements in every script.

Got it :+1:

I tried doing Tixes.ClickDetector, but ClickDetector didn’t show up. That means it doesn’t work (I think)

So I had to loop through Tixes’ children to control its children.

cghhghghcghcghghgghghc What? I think there is a method where you can store the locals in a modulescript. However if you want to keep it in the script then you could space them out. It would make it easier to read as a personal opinion.

1 Like

Exploiters will not be an issue if you handle all the logic on the server and send requests to the client with UI updates. If your logic for the game is on the client, that will be a major issue.

Values that do not change (constants) are things like the valuesFolder.

The location nor the name of the valuesFolder will ever change. There is just not point to put it inside of the playeradded event.

workspace:GetDescendants() loops through the entirety of workspace, which could potentially be massive. If you are doing this a lot, the server will have some issues.

Tix.ClickDetector was just an example. I am unsure if your ClickDetector is actually named ClickDetector. Change it to whatever it is actually named.

Hope that clarified some more.

2 Likes

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.