Is setRGBColor(32,128,192) a valid use of magic number?

Question

According to When is a number a magic number?, I know magic number should not exists, so I know the following code about creating a Label from some UI framework and set the color RGB needs to be improved:

Magic number version:

Label* label=Label::create("someText");
label->setRGBColor(32,128,192);

to refactor it, every number is replaced with a constant:

Non magic number version:

const int LABEL_COLOR_RED=32;
const int LABEL_COLOR_GREEN=128;
const int LABEL_COLOR_BLUE=192;
Label* label=Label::create("someText");
label->setRGBColor(LABEL_COLOR_RED,LABEL_COLOR_GREEN,LABEL_COLOR_BLUE);

However, I found the "Non magic number version" is actually harder to read and maintain because:

1. It needs more number lines of codes

2. It is well known that the first one is red component, second one is green component, third one is blue component. Separating them looks odd and unnatural for me, resulting taking more time to study the code.

3. If multiple UI exists, eg: label2, I may type the wrong variable name and not easily to find, eg:

label_2->setRGBColor(LABEL_2_COLOR_RED,LABEL_2_COLOR_GREEN,LABEL_1_COLOR_BLUE);

4. Designers usually give me 3 new RGB together (eg:12,34,56), instead of just changing R,G or B, keeping label->setRGBColor(r,g,b) is easier to read and modify

5. According to "Comments are a code smell", comments should explain "why" instead of "How". But how can RGB be further explained in "Why"? Just write "//Designer suggest that"? Write "//It needs to show the color red" (isn't specify current result is red bad as const int FOUR=3?)? Or explain what is RGB in details?

6. In my experience, RGB among UI components are almost surely independent to each other, so it is almost surely no 2 UI would share the same "R" (or G,B), even if it is the same currently, it is not likely I would meet the case that "every UI changes the red component from 32 to 128" like it:

const int COMMON_R=32;
Label* label_1=Label::create("someText1");
label_1->setRGBColor(COMMON_R,32,96);
Label* label_2=Label::create("someText2");
label_2->setRGBColor(COMMON_R,128,192);

So I think "magic" color RGB is cleaner and simpler and hence better. Is that true?

Answer 1

Any kind of GUI framework I have seen which uses RGB colors has a datatype for that. So I would recommend to define a constant like

  RGBColor DARKER_BLUE(32,128,192);

and use it accordingly

  label->setRGBColor(DARKER_BLUE);

whenever possible. bdsl's answer mentions this, but where I disagree to that answer is the idea of label->setRGBColor(32,128,192) being generally fine.

In my experience, it is very unlikely a sane, real-world UI framework will only offer a function like setRGBColor with three integer values exclusively, there is typically an overload or variant which can take a color tuple as input. If not, I would probably encapsulate such calls inside some helper function with a descriptive name:

  void SetLabelColorToDarkerBlue(Label *label)
  {
      label->setRGBColor(32,128,192);
  }

Of course, for ergonomically designed UIs, using a single color in only one place is quite rare - normally, you will find the same color reused for several UI elements or a specific set of elements for a certain context.

Hence, either the name of the color constant or the name of the helper function used for setting the color should express the purpose, like SetDefaultLabelColor() or SetDisabledLabelColor(). In case such a function is the one-and-only place in the whole code base where the line label->setRGBColor(32,128,192) occurs, or in case other calls setRGBColor(32,128,192) with exactly the same numbers are intended for a different context, that's ok.

Now, when a designer tells you "all of our warning texts need a color with higher contrast, here are the new RGB values", there should ideally be one-and-only place in the code where this has to be fixed.

You may also consider to use both in conjunction: a context-related function, and a descriptive color constant:

 void SetDefaultLabelColor(Label *label)
 {
     label->setRGBColor(DARKER_BLUE);
 }

Remember what the purpose of avoiding magic numbers is: it is not only to make code more descriptive, but also to make the code follow the DRY principle.

Answer 2

Your change gives you zero improvement. Anyone working with RGB colors knows what the numbers mean. Something small means the color component is almost absent, 255 means the color component is as strong as possible. So defining these as constants just before using them is pointless.

What makes sense is having a header file or a configuration file that defines all kinds of colors. For example in C you could write

#define LABEL_COLOR 32, 128, 192

and later

label->setRGBColor (LABEL_COLOR)

Myself I'd prefer a macro and a function that let you set some color object, and not set a color by using three RGB components, so you can use CMY colors, YUV colors, RGBA colors, grayscale colors and so on.

Anyway, you have one #define that is then used by all labels that should have the same color, so you can decide later to change the color of all labels and it is a one line code change. If you have 50 labels in your code with the same color, it requires 50 code changes.

PS. Someone else mentioned something like COLOR_TURQUOISE. TURQUOISE is obviously always the same color and will never need to be configured. But you could #define LABEL_COLOR as COLOR_TURQUOISE to make all your labels TURQUOISE, and change it to #define LABEL_COLOR COLOR_BRIGHT_YELLOW if you change your mind. That may be a minor improvement to readability.

What I have used is a macro defining for example COLOR_YELLOW and some macros that can make any color brighter, darker, more pale and so on which makes it easier to use related colors. On the other hand, if you have graphic designers who insist on giving you RGB values, then that's what you use.

Answer 3

The issue is not with a magic number, the issue is with a magic value. You're fixating on the numeric types but that's not the core issue here.

Focus on RGB(32,128,192) instead of the individual integer values. If it helps, think of this color as a hex string (#2080c0 in this case).

If you can store this as a premade color object, great! If not, an int[] could work but it's a form of primitive obsession so I suggest using a dedicated type instead.

Answer 4

Label* label=Label::create("someText");
label->setRGBColor(32,128,192);

Is good. As the OP says cleaner and simpler and hence better. Trying to apply the rule against magic numbers here just makes the code harder to work with.

If you were going to use that particular colour multiple times, then having it in a variable or constant, ideally with all three parts held together in some structure, might better, but if it's only used once it's fine as it is.

If it was three parts together you could give it a meaningful name like COLOR_TURQUOISE or LABEL_COLOR or something - those names might be helpful. But a name just to show that 32 is going to be the red component of some colour is not really helpful.

Answer 5

You already sense that hard-coding the color can be problematic.

Do you want to recompile and redistribute the program in order to change some color, such as for the label?

If not, then a configuration file or other external configuration capability that holds the colors is the way to remove these magic numbers from your code.  This will potentially allow different users to customize the colors for their monitor and vision.

It might seem like a lot of work for one color, but surely you have more to configure than the one label color.

Answer 6

You're lost in the woods, as others have already pointed out you need to see/consider the bigger picture of your code:

label->setRGBColor(32,128,192);

Might look like "32", "128" and "192" are "magic" numbers but they're really a representation of the RGB values. As you correctly noticed, this is a very well known way of representing colors.

It's absolutely clear from the context that 32 means the red component, 128 the green component and 192 the blue component (unless you're using some kind of exotic framework that has a different order [which would be stupid btw]).

const int LABEL_COLOR_RED=32;
const int LABEL_COLOR_GREEN=128;
const int LABEL_COLOR_BLUE=192;
Label* label=Label::create("someText");
label->setRGBColor(LABEL_COLOR_RED,LABEL_COLOR_GREEN,LABEL_COLOR_BLUE);

Not only is this code hard to read, it's also more error prone since you have to pass the correct constant to the setRGBColor call (as you pointed out in your question).

I see absolutely no problem with label->setRGBColor(32,128,192); and I think this is the most clean and terse way of specifying a color for a label.

IF you're using the same color in different places, it might be beneficial to replace the integer constants with a variable.

Or create a color (if possible) object that you can pass instead of a list of integers.