Code feeling cluttered

Help and Feedback Code Review
#1

Hello! I just made a really awesome rift opening script that works as intended. Although, as I keep looking at it, it just seems a bit cluttered. A bit messy. Is this code okay as it is, or should I make it more proper and organized?

SCRIPT:

local riftFolder = script.Parent
local stand = riftFolder.Stand
local groundParticles = riftFolder.Part
local secretButton = riftFolder.Clickable

--//RIFT
local model = riftFolder.WOW
local crack = model.crack
local crackPrompt = crack.ProximityPrompt
local particle = model.particle
local light = particle.PointLight
local amongus = model.amongus.SurfaceGui

--//BEAM
local beam = model.BEAM
local plunge = beam.Plunge
local source = beam.Source
local water = beam.Water

--//PARTICLES
local rockEmitter = groundParticles.rockEmitter
local dustEmitter = groundParticles.dustEmitter
local portal1 = particle.portal
local portal2 = particle.pulse

--//SOUND_STAND
local crumble = groundParticles.hatdog

--//SOUND_RIFT
local pulse = particle.beat
local ambience = particle.ambience
local crunch = crack.crunchy
local crunch2 = crack.crunchy2
local rumble = crack.rumble

--//SOUND_BREAK
local SWING = script.hammer
local SWING2 = script.hammer2
local BREAK = particle.HIT
local SHATTER = particle.shatter
local RIFT_OPEN = particle.rift

--//SERVICES
local players = game:GetService("Players"):GetChildren()
local shakeEvent = game:GetService("ReplicatedStorage").thugShake
local ts = game:GetService("TweenService")
local inf = 999999*999999

--//ONGOING
wait(3)
crunch2:Play()
ts:Create(crack, TweenInfo.new(0.4), {Size = Vector3.new(0.2,0.2,0.2)}):Play()
wait(2)
crunch:Play()
ts:Create(crack, TweenInfo.new(0.75), {Size = Vector3.new(1,1,1)}):Play()
wait(1)
secretButton.Transparency = 0
secretButton.ClickDetector.MaxActivationDistance = 7.5
rumble:Play()

--//STAND_RISE
secretButton.ClickDetector.MouseClick:Connect(function()
	wait(1)
	crumble:Play()
	rockEmitter.Enabled = true
	dustEmitter.Enabled = true
	ts:Create(stand.Union, TweenInfo.new(5), {CFrame = stand.Union.CFrame * CFrame.new(4,0,0)}):Play()
	wait(5)
	rockEmitter.Enabled = false
	dustEmitter.Enabled = false
	crumble:Stop()
end)

crackPrompt.Triggered:Connect(function()
	local tweens = {
		ts:Create(light, TweenInfo.new(1), {Brightness = 2.5, Range = 45, Color = Color3.fromRGB(255, 180, 60)}),
		ts:Create(amongus.Parent, TweenInfo.new(1), {Size = Vector3.new(500, 0.05, 500)}),
		ts:Create(amongus.ImageLabel, TweenInfo.new(1), {ImageTransparency = 1}),
		ts:Create(plunge, TweenInfo.new(5), {CFrame = plunge.CFrame * CFrame.new(0,1500,0)}),
		ts:Create(source, TweenInfo.new(5), {CFrame = source.CFrame * CFrame.new(0,-1500,0)})
	}
	crackPrompt.Enabled = false
	SWING:Play()
	wait(1)
	SWING2:Play()
	RIFT_OPEN:Play()
	wait(0.5)
	BREAK:Play()
	SHATTER:Play()
	ts:Create(crack, TweenInfo.new(0.25), {Size = Vector3.new(0.001,0.001,0.001)}):Play()
	wait(0.25)
	shakeEvent:FireAllClients()
	crack:Remove()
	portal1.Enabled = true
	portal2.Enabled = true
	amongus.Enabled = true
	water.Enabled = true
	for i, tween in pairs(tweens) do
		tween:Play()
	end
	ambience:Play()
	pulse:Play()
end)
4 Likes
#2

The code doesn’t look that bad (cluttered) in my opinion but there’s room for improvement (optimization):

  1. Use task.wait instead of wait
  2. Your code creates tweens every time when crackPrompt is triggered. This event could run faster if the tweens were created before this event.
  3. The same issue (point 2) has ClickDetector.MouseClick event - declare tweens outside of this event.

I know that You didn’t ask for that but I wanted to help with making better code. I know that my tips might make the script even more cluttered but in my opinion it won’t be that bad.

4 Likes
#3

math.huge can be used instead of 99999*99999, thats all i can think of

i also tend to leave a gap before and after waits, for readability

TweenInfo.new() may default to a duration of 1 anyway so theres no need to type 1… maybe

Sometimes for related variables with small names and paths, i reduce line numbers like so:

local rockEmitter, dustEmitter = groundParticles.rockEmitter, groundParticles.dustEmitter
local portal1, portal2 = particle.portal, particle.pulse

--//SOUND_STAND
local crumble = groundParticles.hatdog

--//SOUND_RIFT
local pulse = particle.beat
local ambience = particle.ambience
local crunch, crunch2  = crack.crunchy, crack.crunchy2
local rumble = crack.rumble

--//SOUND_BREAK
local SWING, SWING2 = script.hammer, script.hammer2
local BREAK = particle.HIT
local SHATTER = particle.shatter
local RIFT_OPEN = particle.rift

Weather or not its more readable is debateable, I use it more when theres lots of similar small variables.

Ok, im, rlly board so, because i have nothing better to do, here’s almost exactly how i’d format it. Weather or not it is tidier is, debatable, and you may hate it or find it interesting, but here goes (ok its virtually the same-):

--//SERVICES
local RS = game:GetService('ReplicatedStorage')
local TS = game:GetService('TweenService')

local shakeEvent = RS.thugShake

--//CONSTANTS
local inf = math.huge
local oneVector = Vector3.new(1,1,1)
local heightVector = Vector3.new(0,1500,0)

local riftFolder = script.Parent
local stand = riftFolder.Stand
local groundParticles = riftFolder.Part
local secretButton = riftFolder.Clickable

--//RIFT
local model = riftFolder.WOW
local crack = model.crack
local particle = model.particle
local amongus = model.amongus.SurfaceGui

--//BEAM
local beam = model.BEAM
local plunge, source = beam.Plunge, beam.Source

--//PARTICLES
local rockEmitter, dustEmitter = groundParticles.rockEmitter, groundParticles.dustEmitter
local portal1, portal2 = particle.portal, particle.pulse

--//SOUND_STAND
local crumble = groundParticles.hatdog

--//SOUND_RIFT
local pulse, ambience = particle.beat, particle.ambience
local crunch, crunch2 = crack.crunchy, crack.crunchy2
local rumble = crack.rumble

--//SOUND_BREAK
local SWING, SWING2 = script.hammer, script.hammer2
local BREAK, SHATTER, RIFT_OPEN = particle.HIT, particle.shatter, particle.rift

--//ONGOING
task.wait(3)

crunch2:Play()
TS:Create(crack, TweenInfo.new(.4), {Size = oneVector*.2}):Play()

task.wait(2)

crunch:Play()
TS:Create(crack, TweenInfo.new(.75), {Size = oneVector}):Play()

task.wait(1)

secretButton.Transparency = 0
secretButton.ClickDetector.MaxActivationDistance = 7.5
rumble:Play()

--//STAND_RISE
secretButton.ClickDetector.MouseClick:Connect(function()
	task.wait(1)
	
	crumble:Play()
	rockEmitter.Enabled, dustEmitter.Enabled = true, true
	
	local tween = TS:Create(stand.Union, TweenInfo.new(5), {CFrame = stand.Union.CFrame + Vector3.new(1, 0, 0)})
	tween:Play()
	tween.Completed:Wait()
		
	rockEmitter.Enabled, dustEmitter.Enabled = false, false
	crumble:Stop()
end)

crack.ProximityPrompt.Triggered:Connect(function()
	local tweens = {
		TS:Create(particle.PointLight,	TweenInfo.new(),		{Brightness = 2.5, Range = 45, Color = Color3.fromRGB(255, 180, 60)}),
		TS:Create(amongus.Parent,		TweenInfo.new(),		{Size = Vector3.new(500, .05, 500)}),
		TS:Create(amongus.ImageLabel,	TweenInfo.new(),		{ImageTransparency = 1}),
		TS:Create(plunge,				TweenInfo.new(5),	{CFrame = plunge.CFrame + heightVector}),
		TS:Create(source,				TweenInfo.new(5),	{CFrame = source.CFrame - heightVector})
	}
	
	crack.ProximityPrompt.Enabled = false
	SWING:Play()
	
	task.wait(1)
	
	SWING2:Play()
	RIFT_OPEN:Play()
	
	task.wait(.5)
	
	BREAK:Play()
	SHATTER:Play()
	
	local tween = TS:Create(crack, TweenInfo.new(.25), {Size = oneVector*.001})
	tween:Play()
	tween.Completed:Wait()
	
	crack:Remove()
	shakeEvent:FireAllClients()
	portal1.Enabled, portal2.Enabled = true, true
	amongus.Enabled, beam.Water.Enabled = true, true
	
	for _, tween in tweens do
		tween:Play()
	end
	
	ambience:Play()
	pulse:Play()
end)

You should still make the tweens outside the events however, but i wasnt sure if you just wanted it to run once so i didnt do that.

If you do wish to only make it run once however, use :Once() instead of :Connect() where appropriate.

3 Likes
#4

Looks good, I also noticed you have a lot of Instance variables. Now, I don’t know if this is an efficient method but when I have a lot of Instance variables I usually run a for loop for GetDescendants() and store all descendants into a table, I also give every important Instance an unique name so there wouldn’t be any Instances that I need to modify with a similar name, ex:

local Parent = script.Parent;
local Instances = {}

for _, instance in pairs(Parent:GetDescendants()) do
	Instances[instance.Name] = instance;
end;

When I do this my code looks way less cluttered and objects are way easier accessible.
I also don’t have to worry about the instance’s directories nor writing in new instances.

3 Likes
#5

Thank you all for the advice! I’ll be sure to add this in my script and more! :heart::heart::heart:

2 Likes