I wanted to try making neon lights on a tower change color based on a table of BrickColors I've set up, as well as rotate down and up the array so that the table doesn't abruptly move from color [5] to [1]. It's not doing anything at all however, so I hope you guys can help me optimize my code.
local tower = script.Parent local lightFolder = tower.Lights local allLights = lightFolder:GetChildren() local lightValue = nil local lightUp = true colorTable = {BrickColor.new(1029), BrickColor.new(226), BrickColor.new(334), BrickColor.new(24), BrickColor.new(333)} -- "Pastel Yellow "Cool Yellow" "Daisy Yellow" "Bright Yellow" "Gold" while wait(2) do if lightValue == nil then lightValue = 1 elseif lightValue == not nil and lightUp == true then for _, light in pairs(allLights) do light.BrickColor = colorTable[lightValue] end if lightValue then lightValue = lightValue + 1 end if lightValue == 5 then lightUp = false end elseif lightValue == not nil and lightUp == false then for _, light in pairs(allLights) do light.BrickColor = colorTable[lightValue] end if lightValue then lightValue = lightValue - 1 end if lightValue == 1 then lightUp = true end end end
not nil
doesn't make sense. That' just true
. a == not nil
does not mean "a is different from nil".== true
or == nil
ever. Just if something then
or if not something then
suffices.
elseif
when you mean else
. Similarly, don't check conditions are the opposite of what they were in the first. That's the point of else
and elseif
lightValue
should ever be nil
. Start at 1
.colors
instead of colorTable
-- saying "table" doesn't mean anything, it's just a filler wordAfter beginning with lightValue = 1
and renaming colorTable
to colors
, we get this script:
local tower = script.Parent local lightFolder = tower.Lights local allLights = lightFolder:GetChildren() local lightValue = 1 local lightUp = true colors = { BrickColor.new("Pastel yellow"), BrickColor.new("Cool yellow"), BrickColor.new("Daisy yellow"), BrickColor.new("Bright yellow"), BrickColor.new("Gold"), } while wait(2) do if lightUp == true then -- bad! don't say `== true`. `lightUp` on its own is the condition! for _, light in pairs(allLights) do light.BrickColor = colors[lightValue] end lightValue = lightValue + 1 if lightValue == 5 then lightUp = false end elseif lightUp == false then -- bad! we just mean `else` for _, light in pairs(allLights) do light.BrickColor = colors[lightValue] end lightValue = lightValue - 1 if lightValue == 1 then lightUp = true end end end
Fixing those two conditions,
local tower = script.Parent local lightFolder = tower.Lights local allLights = lightFolder:GetChildren() local lightValue = 1 local lightUp = true colors = { BrickColor.new("Pastel yellow"), BrickColor.new("Cool yellow"), BrickColor.new("Daisy yellow"), BrickColor.new("Bright yellow"), BrickColor.new("Gold"), } while wait(2) do if lightUp then for _, light in pairs(allLights) do light.BrickColor = colors[lightValue] end lightValue = lightValue + 1 if lightValue == 5 then -- bad! magic value. lightUp = false end else for _, light in pairs(allLights) do light.BrickColor = colors[lightValue] end lightValue = lightValue - 1 if lightValue == 1 then lightUp = true end end end
You have a "magic value" in your code. Why is that 5
as opposed to 4
or 6
? In this case, it's the number of colors there are, #colors
.
That number is in fact wrong -- you'll be skipping the last. Similarly, == 1
is wrong.
The fixed final version looks like this:
local tower = script.Parent local lightFolder = tower.Lights local allLights = lightFolder:GetChildren() local lightValue = 0 local lightUp = true colors = { BrickColor.new("Pastel yellow"), BrickColor.new("Cool yellow"), BrickColor.new("Daisy yellow"), BrickColor.new("Bright yellow"), BrickColor.new("Gold"), } while wait(2) do if lightUp then lightValue = lightValue + 1 for _, light in pairs(allLights) do light.BrickColor = colors[lightValue] end if lightValue == #colors then lightUp = false end else lightValue = lightValue - 1 for _, light in pairs(allLights) do light.BrickColor = colors[lightValue] end if lightValue == 1 then lightUp = true end end end
Clean code reads like prose. You should start describing simply what you want.
You wanted to:
So here is the code that does exactly that:
function setColor(color) for _, light in pairs(allLights) do light.BrickColor = color end end colors = { BrickColor.new("Pastel yellow"), BrickColor.new("Cool yellow"), BrickColor.new("Daisy yellow"), BrickColor.new("Bright yellow"), BrickColor.new("Gold"), } while true do wait(2) setColor(colors[1]) wait(2) setColor(colors[2]) wait(2) setColor(colors[3]) wait(2) setColor(colors[4]) wait(2) setColor(colors[5]) wait(2) setColor(colors[4]) wait(2) setColor(colors[3]) wait(2) setColor(colors[2]) end
This is shorter than your original code and very clear, but very repetitive. However, it clearly shows the pattern: 1 2 3 4; 5 4 3 2; 1 2 3 4; 5 4 3 2.
This can be easily done with a for
loop:
local allLights = lightFolder:GetChildren() function setColor(color) for _, light in pairs(allLights) do light.BrickColor = color end end colors = { BrickColor.new("Pastel yellow"), BrickColor.new("Cool yellow"), BrickColor.new("Daisy yellow"), BrickColor.new("Bright yellow"), BrickColor.new("Gold"), } while true do for i = 1, #colors - 1 do wait(2) setColor(colors[i]) end for i = #colors, 2, -1 do wait(2) setColor(colors[i]) end end
This is much simpler, because it's just doing precisely what you wanted. Don't be clever!