Answered by
8 years ago Edited 8 years ago
Advice / Problems
not nil
doesn't make sense. That' just true
. a == not nil
does not mean "a is different from nil".
- Don't use
== true
or == nil
ever. Just if something then
or if not something then
suffices.
- Don't use
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
- Don't mix different types of things together. There's no reason that
lightValue
should ever be nil
. Start at 1
.
- Use a name like
colors
instead of colorTable
-- saying "table" doesn't mean anything, it's just a filler word
- Use the names for the BrickColor values. That lets you get rid of the comment -- the code can be understood on its own!
Cleaned-up
After beginning with lightValue = 1
and renaming colorTable
to colors
, we get this script:
01 | local tower = script.Parent |
02 | local lightFolder = tower.Lights |
03 | local allLights = lightFolder:GetChildren() |
09 | BrickColor.new( "Pastel yellow" ), |
10 | BrickColor.new( "Cool yellow" ), |
11 | BrickColor.new( "Daisy yellow" ), |
12 | BrickColor.new( "Bright yellow" ), |
13 | BrickColor.new( "Gold" ), |
17 | if lightUp = = true then |
18 | for _, light in pairs (allLights) do |
19 | light.BrickColor = colors [ lightValue ] |
21 | lightValue = lightValue + 1 |
22 | if lightValue = = 5 then |
25 | elseif lightUp = = false then |
26 | for _, light in pairs (allLights) do |
27 | light.BrickColor = colors [ lightValue ] |
29 | lightValue = lightValue - 1 |
30 | if lightValue = = 1 then |
Fixing those two conditions,
01 | local tower = script.Parent |
02 | local lightFolder = tower.Lights |
03 | local allLights = lightFolder:GetChildren() |
09 | BrickColor.new( "Pastel yellow" ), |
10 | BrickColor.new( "Cool yellow" ), |
11 | BrickColor.new( "Daisy yellow" ), |
12 | BrickColor.new( "Bright yellow" ), |
13 | BrickColor.new( "Gold" ), |
18 | for _, light in pairs (allLights) do |
19 | light.BrickColor = colors [ lightValue ] |
21 | lightValue = lightValue + 1 |
22 | if lightValue = = 5 then |
26 | for _, light in pairs (allLights) do |
27 | light.BrickColor = colors [ lightValue ] |
29 | lightValue = lightValue - 1 |
30 | if lightValue = = 1 then |
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:
01 | local tower = script.Parent |
02 | local lightFolder = tower.Lights |
03 | local allLights = lightFolder:GetChildren() |
09 | BrickColor.new( "Pastel yellow" ), |
10 | BrickColor.new( "Cool yellow" ), |
11 | BrickColor.new( "Daisy yellow" ), |
12 | BrickColor.new( "Bright yellow" ), |
13 | BrickColor.new( "Gold" ), |
18 | lightValue = lightValue + 1 |
19 | for _, light in pairs (allLights) do |
20 | light.BrickColor = colors [ lightValue ] |
22 | if lightValue = = #colors then |
26 | lightValue = lightValue - 1 |
27 | for _, light in pairs (allLights) do |
28 | light.BrickColor = colors [ lightValue ] |
30 | if lightValue = = 1 then |
A better start
Clean code reads like prose. You should start describing simply what you want.
You wanted to:
- Show the 1st color
- Show the 2nd
- ...
- Show the last color
- Show the second to last color
- Show the third to last color
- ...
- Show the second color
- Show the first color
- Show the second color
- ...
So here is the code that does exactly that:
01 | function setColor(color) |
02 | for _, light in pairs (allLights) do |
03 | light.BrickColor = color |
08 | BrickColor.new( "Pastel yellow" ), |
09 | BrickColor.new( "Cool yellow" ), |
10 | BrickColor.new( "Daisy yellow" ), |
11 | BrickColor.new( "Bright yellow" ), |
12 | BrickColor.new( "Gold" ), |
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:
01 | local allLights = lightFolder:GetChildren() |
03 | function setColor(color) |
04 | for _, light in pairs (allLights) do |
05 | light.BrickColor = color |
10 | BrickColor.new( "Pastel yellow" ), |
11 | BrickColor.new( "Cool yellow" ), |
12 | BrickColor.new( "Daisy yellow" ), |
13 | BrickColor.new( "Bright yellow" ), |
14 | BrickColor.new( "Gold" ), |
18 | for i = 1 , #colors - 1 do |
22 | for i = #colors, 2 , - 1 do |
This is much simpler, because it's just doing precisely what you wanted. Don't be clever!