Ugly code and rainbows

Newbie & Debugging Questions
#1

So, i was kinda bored and started working on a rainbow function. Actually, it’s just a function that returns me color based on an integer.

It looks pretty good on screen, quite efficient too (you can puke later)

But the code is ugly and full of magic numbers; Even though the colors are perfect (I checked them), it still looks wrong :-\

	public int colorChart(int color) {
		color = color % 256;
		int rcolor;
		
		if (color <= 42) {
			rcolor = (0xFF << 16) + ((6 * color) << 8);
		}
		else if (color > 42 && color <= 84) {
			rcolor = (0xFF - (6 * (color - 43)) << 16) + (0xFF << 8);
		}
		else if (color > 84 && color <= 127) {
			rcolor = (0xFF << 8) + (6 * (color - 85));
		}
		else if (color > 127 && color <= 169) {
			rcolor = (0xFF - (6 * (color - 128)) << 8) + 0xFF;
		}
		else if (color > 169 && color <= 212) {
			rcolor = 0xFF + ((6 * (color - 170)) << 16);
		}
		else {
			rcolor = (0xFF << 16) + (0xFF - (6 * (color - 213)));
		}		
			
		return rcolor;
	}

The prototypes weren’t worse tho, i miscalculated the number of colors and the result was a kinda-LSD type illusion. My head still aches.
Maybe that is causing some sort of brain blockage, because i don’t know how to make the code any better

Ideas? Thanks. :wink:

#2

Before looking at your code in depth…what exactly is wrong?

#3

Nothing at all, i just thought i could have used a different design pattern

The thing is that i just hate to hard code numbers, even on conditional statements

#4

If it works…don’t mess with it ;D

#5

The code itself looks fine, I’d only suggest one improvement: You could store the magic constants like 42, 84, 127 etc in an array. That lets you do two things: one, if you change the numbers, you don’t have to worry about forgetting to update the code in three different places per number. Two, it would let you tweak the array itself with another function in order to animate the effect.

#6

As a veteran of many years of coding it looks great to me.

Your code is a pure function i.e. given the same inputs it always gives the same output. That’s a good thing and it’s easy to read and understand. You could come back to this function years from now and it won’t take you long to figure out what’s going on.

If you’re happy with the output then no need to change anything.

[edit] if you insist on some changes then: add a one-liner comment saying this produces a rainbow and you could make the function static.

#7

What you have there is not actually a linear hue gradient. You should take a look at HSL (not HSV):


You can simply ‘pick’ a Hue by specifying an angle.

As you only have 256 possible values in your current code, you can optionally create a lookup table.
IMHO, with HSL you’ll endup with much better code, and no magic constants whatsoever.

#8

That is really interesting, Riven, i’ll take a look

Thank you guys :smiley:

#9

It’s not late to polish your code after everything run and ready to published.