New to scripting, any critiques or tips on my script?

Help and Feedback Creations Feedback
#1

I’ve been scripting for around a month or two now, and I’m not sure if I’m improving or not. I’ve been reading and watching a lot of tutorials and I just feel like I get stuck a lot, I understand that scripting will take awhile to get better at but I believe direct feedback will allow me to progress and learn better

This code is in a LocalScript under StarterPlayerScripts. Basically what it does is whenever a player touches a spawner within the game, the spawner will fire to the client which course, hole, and direction the ball should be spawned at, the spawner will then turn red signaling that the ball has been spawned and can only despawn the ball if the ball is put in the corresponding hole or the player cancels the hole by clicking the spawner while it’s red. The game I’m trying to make is a very very simple golf game where a player has a tool that only collides with a ball and the player has to try to get the ball into the hole. There’s a bunch of other stuff but that’s not relevant to the script.

--*&@&*--

--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- Variables

--=-=-=-=-=-=-=-- Services
local replicatedStorage = game:GetService("ReplicatedStorage")
local physicsService = game:GetService("PhysicsService")
local tweenService = game:GetService("TweenService")
--=-=-=-=-=-=-=-- Services

--=-=-=-=-=-=-=-- Instances & Events
local spawnEvent = replicatedStorage.RemoteEvents:WaitForChild("SpawnEvent")
local ball = replicatedStorage.Instances:WaitForChild("Ball")
local plr = game.Players.LocalPlayer
local plrName = plr.Name
--=-=-=-=-=-=-=-- Instances & Values

--=-=-=-=-=-=-=-- Config
local tweenTime = 2
local spawnOffset = 2.5
local maxActivationDistance = 50
local cancelCooldown = 1
local tweenInfo = TweenInfo.new(tweenTime)
--=-=-=-=-=-=-=-- Config

--=-=-=-=-=-=-=-- Debounces
local canSpawn = true
local inHole = false
local cancelled = false
--=-=-=-=-=-=-=-- Debounces

--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- Variables

--*&@&*--

--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- RemoteEvent fired
spawnEvent.OnClientEvent:Connect(function(c,h,d)
	if canSpawn == true then
		canSpawn = false

		--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- Player touched spawner
		local course = workspace.World.Courses:FindFirstChild("Course"..c)
		if course then
			local hole = course.Holes:FindFirstChild("Hole"..h)
			if hole then
				print("Succesfully loaded Hole"..h)
				
				--=-=-=-=-=-=-=-- Hole variables
				local spawner = hole:FindFirstChild("Spawner")
				local holeDisplay = hole:WaitForChild("HoleDisplay")
				local display = holeDisplay:WaitForChild("Status")
				local status = display:WaitForChild("Completed")
				local spawnerColor = spawner.Color
				local clickDetector = spawner:WaitForChild("ClickDetector")
				local highlight = spawner:WaitForChild("Highlight")
				--=-=-=-=-=-=-=-- Hole variables
				
				spawner.Color = Color3.fromRGB(255)
				highlight.Enabled = true
				clickDetector.MaxActivationDistance = maxActivationDistance
				spawner.Spawn:Play()
				local clone = ball:Clone()
				clone.Parent = workspace.ActiveBalls
		--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- Player touched spawner
				
				--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- Checks what direction the ball should spawned in
				if d == "back" then
					clone.Position = spawner.Position + Vector3.new(0,0,spawnOffset)
				elseif d == "front" then
					clone.Position = spawner.Position + Vector3.new(0,0,-spawnOffset)
				elseif d == "right" then
					clone.Position = spawner.Position + Vector3.new(spawnOffset,0,0)
				elseif d == "left" then
					clone.Position = spawner.Position + Vector3.new(-spawnOffset,0,0)
				end
				--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- Checks what direction the ball should spawned in
				
				--=-=-=-=-=-=-=-- Checks if a player cancels
				clickDetector.MouseClick:Connect(function()
					if cancelled == false then
						cancelled = true
						clone:Destroy()
						print("Cancelled Hole"..h)
						spawner.Cancel:Play()
						spawner.Color = spawnerColor
						highlight.Enabled = false
						clickDetector.MaxActivationDistance = 0
						canSpawn = true
						task.wait(cancelCooldown)
						cancelled = false
					end
				end)
				--=-=-=-=-=-=-=-- Checks if a player cancels
				
				--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- Ball made it into hole
				clone.Touched:Connect(function(hit)
					if hit.Name == hole.Name then
						if inHole == false  then
							if status.Value == false then
								status.Value = true
								display.Color = Color3.fromRGB(0,255,0)
							end
							print("Ball made it into Hole"..h)
							inHole = true
							local tween = tweenService:Create(clone,tweenInfo,{Transparency = 1})
							local particle = hit:FindFirstChild("ParticleEmitter")
							local sound = hit:FindFirstChild("Sound")
							clickDetector.MaxActivationDistance = 0
							clone.Color = Color3.fromRGB(0,255)
							clone.Anchored = true
							clone.CanCollide = false
							sound:Play()
							tween:Play()
							particle.Enabled = true
							task.wait(0.5)
							particle.Enabled = false
							task.wait(tweenTime-0.5)
							clone:Destroy()
							inHole = false
							spawner.Color = spawnerColor
							highlight.Enabled = false
							canSpawn = true
						end
					end
				end)
				--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- Ball made it into hole
				
		--=-=-=-=-=-=-=-- Debugging
			else
				warn("Could not find Hole"..h.."!")
				error("There was an error loading specified Hole")
			end
		else
			warn("Could not find Course"..c.."!")
			error("There was an error loading specified Hole")
		end
		--=-=-=-=-=-=-=-- Debugging
		
	end -- If debounce end
end) -- Player added end

--=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-- RemoteEvent fired

--*&@&*--

Any critiques or tips?
I would really appreciate if I could get feedback in anyway possible. Right now the script works fine and as intended, however I feel like it’s super messy and can be optimized much better. Is there any flaws, better ways of doing things, and quicker/easier methods? This script basically sums up most of my scripting knowledge that I’ve learned so far by the way.
To sum it up, I need to know if I can make the script better

Screenshot 2024-03-18 184222
Screenshot 2024-03-18 184222864×658 50.8 KB

#2

for this section of the code

if d == "back" then
					clone.Position = spawner.Position + Vector3.new(0,0,spawnOffset)
				elseif d == "front" then
					clone.Position = spawner.Position + Vector3.new(0,0,-spawnOffset)
				elseif d == "right" then
					clone.Position = spawner.Position + Vector3.new(spawnOffset,0,0)
				elseif d == "left" then
					clone.Position = spawner.Position + Vector3.new(-spawnOffset,0,0)
				end

you can initalize a table before:

positions_offset = {

back = Vector3.new(0,0,1);
front =  Vector3.new(0,0,-1);
right = Vector3.new(1,0,0);
left = Vector3.new(-1,0,0);
}

then back to that segment code, you can replace the whole segment code by only this:

clone.Position = spawner.Position + positions_offset[d]*spawnOffset

it wont ba a major performance boost, but is significant when you have many things like this

2 Likes
#3

Your script is already great, there’s not much to critique except for personal preference. But if I were making the script I would try to avoid nesting whenever possible, for example I can see that you have basically the entire contents of the spawnEvent.OnClientEvent connection inside of a if statement that checks for a debounce. But rather than doing:

if debounce == false then
	debounce = true
	--other code
end

You can just do:

if debounce == true then return end --If this function shouldn't run yet, return so it stops running
debounce = true
--other code

Admittedly, doing it this way makes it a lot more confusing, but it does let you avoid nesting which I find makes code harder to follow.

Aside from that I would also try to avoid the .Touched event whenever possible, I find that it’s pretty inconsistent at times. Instead of a touched event you can use RunService.RenderStepped:Connect() and then loop through the parts returned by workspace:GetPartsInPart(hole) and check if any of them are equal to the cloned ball. This is much worse for performance though, the touched event definitely works fine for this use-case and I don’t recommend using this alternative. I would probably just use it anyways because I’m allergic to the touched event.

Also, I’m not sure if you’re aware of this and as you said the script works so it might not be too big of a deal. But when you create an event connection inside of another event connection, the second event connection stays connected even when the first event connections function ends. That is to say, when you do:

spawnEvent.OnClientEvent:Connect(function(blah)
	clickDetector.MouseClick:Connect(function()
		print("blah")
	end)
end)

Every single time the spawnEvent fires, you create another MouseClick connection. The first time a ball is spawned, there will be 1 MouseClick event for hole.spawner.clickDetector, so it will print “blah” once every time you click. The second time a ball is spawned, you create another MouseClick event for hole.spawner.clickDetector, so it prints “blah” twice. In your case, it just runs the hole cancel code twice, which would immediately error because it can’t destroy the clone twice and so it wouldn’t actually cause any problems. Again, the code works, but it’s definitely something to be aware of in the future. When this does cause problems and you aren’t aware of it, it can be pretty nasty to debug. (Speaking from personal experience, I still have nightmares)

All in all, your code is great. After all, it works, which makes it better than my code.

1 Like
#4

also for this you are continously adding an event for mouse click, now if this section of code runs two times, that mean when the player clicks the mouse once, this will fire twice, you should make a habit to disconnect useless events(as it uses resources), if the clickDetector is being destroyed then no problem it automatically disconnects, but if not then to disconnect its easy:

local clickDetector_clicked
clickDetector_clicked = clickDetector.MouseClick:Connect(function()
					if cancelled == false then
						cancelled = true
						clone:Destroy()
						print("Cancelled Hole"..h)
						spawner.Cancel:Play()
						spawner.Color = spawnerColor
						highlight.Enabled = false
						clickDetector.MaxActivationDistance = 0
						canSpawn = true
						task.wait(cancelCooldown)
						cancelled = false
                        clickDetector_clicked:Disconnect()
					end
				end)
1 Like
#5

thanks a lot! i was wondering why the cancel print was printing twice