How can I improve this horror game environment code?

Help and Feedback Code Review
#1

Hi all! I’m working on a horror game. This script basically turns the lights off every hour. Any ways you can think of to improve it?

local lights = workspace.Lights
while true do
	wait(3600)
	workspace.Powerdown:Play()
	for i,v in pairs(lights:GetChildren()) do
		for i,j in pairs(v.Model:GetChildren()) do

			local spotlight = j:FindFirstChildWhichIsA("SpotLight")
			if spotlight then
				spotlight.Enabled = false
			end
			local hum = j:FindFirstChildWhichIsA("Sound")
			if hum then
				hum.Playing = false
			end
			j.BrickColor = BrickColor.new("Smoky grey")
			j.Material = "Plastic"
		end
	end
	game.Workspace.Evil.Playing = true
	wait(5)
	workspace.rebooting:Play()
	wait(5)
	game:GetService("TweenService"):Create(game.Lighting,TweenInfo.new(20),{OutdoorAmbient = Color3.fromRGB(255,0,0)}):Play()
	game:GetService("TweenService"):Create(workspace.Evil,TweenInfo.new(10),{Volume = 2}):Play()
	wait(295)
	game:GetService("TweenService"):Create(game.Lighting,TweenInfo.new(10),{OutdoorAmbient = Color3.fromRGB(255,0,0)}):Play()
	game:GetService("TweenService"):Create(workspace.Evil,TweenInfo.new(10),{Volume = 0}):Play()
	wait(10)
	game.Workspace.Evil.Playing = false
	for i,v in pairs(lights:GetChildren()) do
		for i,j in pairs(v.Model:GetChildren()) do
			local spotlight = j:FindFirstChildWhichIsA("SpotLight")
			if spotlight then
				spotlight.Enabled = true
			end
			local hum = j:FindFirstChildWhichIsA("Sound")
			if hum then
				hum.Playing = true
			end
			j.BrickColor = BrickColor.new("Burlap")
		j.Material = "Neon"
		end
	end
end
#2

use task.wait()

why not use :GetDescendants()

you have repeated code here, try using variables

4 Likes
#3

Will try using these. Thank you!

#4

  • Variable like i, v, j are too vague. Try naming them according to the context

  • Spell out words fully. Abbreviations generally make code easier to write, but harder to read.

  • j.Material = "Neon" Use enum instead

2 Likes
#5

First time posting here, it’s fine if I just remake and comment everything in the script right?

local Lighting = game:GetService("Lighting")
local TweenService = game:GetService("TweenService") -- Services are at the very top (notice capitals), then tables, then variables, and finally constants in general

local spotlights = {} -- It's best to store these instead of searching for them each time, refer to loops below
local sounds = {}
local parts = {}

local powerSound = workspace.Powerdown
local evilMusic = workspace.Evil -- Using variables lets you easily change the entire script at once
local rebootSound = workspace.rebooting -- You should consider making the sound capitalized

local OFF_COLOR = BrickColor.new("Smoky grey") -- These will never change through the script so use camel case
local ON_COLOR = BrickColor.new("Burlap")

local TWEEN_20 = TweenInfo.new(20) -- Unnecessary but I'm lazy
local TWEEN_10 = TweenInfo.new(10) -- I grouped the tween stuff together
local ambientIn = TweenService:Create(Lighting, TWEEN_20, {OutdoorAmbient = Color3.fromRGB(255,0,0)})
local ambientOut = TweenService:Create(Lighting, TWEEN_10, {OutdoorAmbient = Color3.fromRGB(255,0,0)})
local musicIn = TweenService:Create(evilMusic, TWEEN_10, {Volume = 2})
local musicOut = TweenService:Create(evilMusic, TWEEN_10, {Volume = 0}) -- It's best to create the tween only once

for _, v in next, workspace.Lights:GetChildren() do -- _ is empty variable, micro-optimization. I do it since i is not in use
	for _, part in next, v.Model:GetChildren() do -- Next is just preference, you can use pairs()
		local spotlight = part:FindFirstChildWhichIsA("SpotLight")
		if spotlight then
			table.insert(spotlights, spotlight)
		end
		
		local sound = part:FindFirstChildWhichIsA("Sound")
		if sound then
			table.insert(sounds, sound)
		end
		
		table.insert(parts, part)
	end
end

while task.wait(3600) do -- Replace true with task.wait()
	powerSound:Play()
	
	for _, v in next, spotlights do
		v.Enabled = false
	end
	for _, v in next, sounds do
		v.Playing = false
	end
	for _, v in next, parts do
		v.BrickColor = OFF_COLOR
		v.Material = Enum.Material.SmoothPlastic
	end
	
	evilMusic.Playing = true
	task.wait(5)
	
	rebootSound:Play()
	task.wait(5)
	
	ambientIn:Play()
	musicIn:Play()
	task.wait(295)
	
	ambientOut:Play()
	musicOut:Play()
	task.wait(10)
	
	evilMusic.Playing = false
	for _, v in next, spotlights do
		v.Enabled = true
	end
	for _, v in next, sounds do
		v.Playing = true
	end
	for _, v in next, parts do
		v.BrickColor = ON_COLOR
		v.Material = Enum.Material.Neon
	end
end
2 Likes
#6

Thank you so much! Is there any reason for task.wait()? I’ve gotten suggested to use it a lot, but I have no idea what’s different about it.

#7

task.wait() is the improved version of wait() that was released with the new Task System from Roblox. You can read about the differences there.

The main difference is that task.wait() is not limited to 1 / 30 of a second. It can get to speeds of 1 / 60 or even faster depending on when it is called.

1 Like
#8

:GetDescendants() could still be used here

1 Like
#9

I didn’t know how the structure was set up so I didn’t use it. There could be lots of extra instances that would slow down :GetDescendants().

1 Like
#10

looking at the code each instance under Lights has a model parented under them, and you basically loop through every instance in those models

the only thing you would go through unnecessarily would be the spotlights and sounds, so yea I guess :GetChildren() is would have more performance here

my bad

#11

Thank you for the explanation!

#12

There’s around 50 different lights in a folder called Lights in workspace if that clears anything up. The light model itself opens into another model containing 4 parts, and one of those parts has the spotlight and the sound.

#13

well I basically answered my own question