# Questions re comments in Kaleidoscope-LEDEffect-Rainbow.{h,cpp}
**URL:** https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866
**Category:** Plugins
**Created:** [August 1, 2018, 2:20pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866 "2018-08-01T14:20:08Z")
**Posts on this page:** 12
**Page:** 1
### Author:  [@jamadagni](https://community.keyboard.io/u/jamadagni)
#### Post date: [August 1, 2018, 2:20pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/1 "2018-08-01T14:20:08Z")
My questions are re [these lines in Kaleidoscope-LEDEffect-Rainbow.h](https://github.com/keyboardio/Kaleidoscope-LEDEffect-Rainbow/blob/master/src/Kaleidoscope-LEDEffect-Rainbow.h#L22):
```auto
uint16_t rainbow_hue = 0; // stores 0 to 614
uint8_t rainbow_steps = 1; // number of hues we skip in a 360 range per update
```
However comparing to [the actual usage in Kaleidoscope-LEDEffect-Rainbow.cpp](https://github.com/keyboardio/Kaleidoscope-LEDEffect-Rainbow/blob/master/src/Kaleidoscope-LEDEffect-Rainbow.cpp#L15):
```auto
rainbow_hue += rainbow_steps;
if (rainbow_hue >= 255) {
rainbow_hue -= 255;
}
```
It seems that the comments are somewhat misleading because the hue value can never exceed 255. Perhaps the comments are a leftover from another context?
---
### Author:  [@jamadagni](https://community.keyboard.io/u/jamadagni)
#### Post date: [August 1, 2018, 3:20pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/2 "2018-08-01T15:20:56Z")
In fact it would seem that by simply changing the type of hue to 8-bit rather than 16-bit, one could simply remove the if block, because addition automatically wraps around…
---
### Author:  [@bagels](https://community.keyboard.io/u/bagels)
#### Post date: [August 1, 2018, 11:11pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/3 "2018-08-01T23:11:38Z")
You can probably do just that. We also would like to use default values in the constructor so that we can get rid of the extra constructors.
---
### Author:  [@jamadagni](https://community.keyboard.io/u/jamadagni)
#### Post date: [August 1, 2018, 11:42pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/4 "2018-08-01T23:42:14Z")
> [@bagels](#):
>
> We also would like to use default values in the constructor so that we can get rid of the extra constructors.
What extra constructors? I only see one!
---
### Author:  [@bagels](https://community.keyboard.io/u/bagels)
#### Post date: [August 1, 2018, 11:53pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/5 "2018-08-01T23:53:33Z")
Ah, my mistake, I believe at one point there were more. Sorry for the confusion!
---
### Author:  [@jamadagni](https://community.keyboard.io/u/jamadagni)
#### Post date: [August 2, 2018, 12:01pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/6 "2018-08-02T12:01:20Z")
Another question. [This line](https://github.com/jamadagni/Kaleidoscope-LEDEffect-Rainbow/blob/8a77cdaaff0a55019a83b3bcc1b3341baed264b4/src/Kaleidoscope-LEDEffect-Rainbow.h#L24):
```auto
uint8_t rainbow_steps = 1; // number of hues we skip in a 360 range per update
```
and [this one](https://github.com/jamadagni/Kaleidoscope-LEDEffect-Rainbow/blob/8a77cdaaff0a55019a83b3bcc1b3341baed264b4/src/Kaleidoscope-LEDEffect-Rainbow.h#L50):
```auto
uint8_t rainbow_wave_steps = 1; // number of hues we skip in a 360 range per update
```
why are these lines even present? They are only used [thus](https://github.com/jamadagni/Kaleidoscope-LEDEffect-Rainbow/blob/master/src/Kaleidoscope-LEDEffect-Rainbow.cpp#L15):
```auto
rainbow_hue += rainbow_steps;
```
and [thus](https://github.com/jamadagni/Kaleidoscope-LEDEffect-Rainbow/blob/8a77cdaaff0a55019a83b3bcc1b3341baed264b4/src/Kaleidoscope-LEDEffect-Rainbow.cpp#L49):
```auto
rainbow_hue += rainbow_wave_steps;
```
Neither are they modified anywhere in the program nor are they exposed to the user to be configurable. I’ve submitted a PR with some cleanups and can add a commit either exposing this to the user or conversely just integrating the constant into the code.
---
### Author:  [@jamadagni](https://community.keyboard.io/u/jamadagni)
#### Post date: [August 2, 2018, 12:04pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/7 "2018-08-02T12:04:18Z")
> [@bagels](#):
>
> , I believe at one point there were more.
Looks like you’re talking about [this](https://github.com/keyboardio/Kaleidoscope-LEDEffect-Rainbow/pull/2/commits/cbee57759cb20aa2380dbbc8540da1d37049a043)…
---
### Author:  [@algernon](https://community.keyboard.io/u/algernon)
#### Post date: [August 2, 2018, 1:19pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/8 "2018-08-02T13:19:12Z")
> [@jamadagni](#):
>
> Neither are they modified anywhere in the program nor are they exposed to the user to be configurable.
Code clarity, mostly. `rainbow_hue += rainbow_steps` is easier to understand than `rainbow_hue += 1` - no magic constants. Mind you, marking this `const` or something may help, but I’d keep it for the name, and let the compiler do the inlining.
---
### Author:  [@jamadagni](https://community.keyboard.io/u/jamadagni)
#### Post date: [August 2, 2018, 2:26pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/9 "2018-08-02T14:26:13Z")
> [@algernon](#):
>
> `rainbow_hue += rainbow_steps` is easier to understand than `rainbow_hue += 1` - no magic constants.
Aw come on! 1 is a “magic constant”? `hue += 1` clearly means “go to next hue”!
---
### Author:  [@algernon](https://community.keyboard.io/u/algernon)
#### Post date: [August 2, 2018, 2:46pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/10 "2018-08-02T14:46:19Z")
> [@jamadagni](#):
>
> `hue += 1` clearly means “go to next hue”!
Only until we don’t ever want to change it, to speed things up, for example. Then `hue++` would be fine. If we want to keep changing it an option, `hue += step` feels - to me - nicer. Under the hood, they can likely compile to the same thing, but the `+= step` variant is a tiny bit easier to change later on, or to make public, if so desired.
On the other hand, this is not a very strong opinion, so if @jesse is fine with just inlining, that works for me as well.
---
### Author:  [@jesse](https://community.keyboard.io/u/jesse)
#### Post date: [August 2, 2018, 3:55pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/11 "2018-08-02T15:55:10Z")
It’s my preference to not throw away the steps variables. We’ve had different default values at different times, there’s no run time overhead and I prefer the explicitness.
---
### Author:  [@jamadagni](https://community.keyboard.io/u/jamadagni)
#### Post date: [August 2, 2018, 4:12pm UTC](https://community.keyboard.io/t/questions-re-comments-in-kaleidoscope-ledeffect-rainbow-h-cpp/1866/12 "2018-08-02T16:12:04Z")
Ohkay, so then we’ll expose it as a signed integer so people can have reverse rainbows too!