18

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);
  1. 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

  2. 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?

  3. 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?

10
  • 37
    Are you able to create some kind of Color object, and pass that to label?
    – Caleth
    Aug 24 at 8:11
  • 6
    What happens when you have 50 labels in your code and you need to change the colour of all of them?
    – pjc50
    Aug 24 at 9:51
  • 3
    Whether a magic number or a named constant, having a colour hardcoded is generally a pretty bad idea that will likely lead to complaints that your app does not support custom colour schemes (or the watered-down version of those: dark mode). Aug 24 at 17:38
  • 1
    The three individual colors (to me) are magic. Red in isolation, green in isolation, and blue in isolation. Even seeing the numbers doesn't really tell me what the composite color is. some_kind_of_purple = Color(32, 128, 192) tells me a LOT more. No comment. But a named triplet. Or perhaps default_label_color = Color(1, 2, 3)
    – Steve
    Aug 25 at 15:57
  • 1
    @Steve What's magic about a "Color"? I'm a seasoned web developer and I don't think any color has any magic to it. Unless you're using it as a sentinel value, which I initially thought this question was about.
    – Marco
    Aug 26 at 10:44

6 Answers 6

66

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.

10
  • 25
    I agree entirely, and I think the phrase "magic numbers" is sometimes unhelpful, because what we really mean is "magic values" - things that should be centralised somewhere, but are not.
    – IMSoP
    Aug 24 at 16:19
  • 1
    It's probably a bad idea to use "darker blue" as the colour name; in most cases you want semantic naming, e.g. "Label Colour" so that you can consistently recolour the site. Aug 25 at 6:38
  • 13
    @JackAidley: ideally, you use both - a name like "DarkerBlue" for the color, and a semantic name RGBColor LabelColor=DarkerBlue; The color name for readability, and the semantic name for having a place for allowing central changes
    – Doc Brown
    Aug 25 at 6:41
  • 5
    "there should ideally be one-and-only place in the code where this has to be fixed" I would say there should be only on place, but it doesn't have to be in code. Having all colors in a theme config file could be convenient.
    – kapex
    Aug 25 at 9:44
  • 2
    Obligatory XKCD for colour name inspiration (they're hex codes, but the conversion process is probably easy enough).
    – Pam
    Aug 25 at 9:52
19

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.

3
  • 7
    That two-stage mapping of colours is often useful in web design, as well. It's often useful to first define some primary and secondary brand colours, then define categories of UI element as being in each of those colours. Then the actual CSS rules only ever specify which category they belong to, never the actual colour to use.
    – IMSoP
    Aug 24 at 16:22
  • I had a project with two customers who wanted a "skinned" version with different colors. Just about managed to stop someone from changing the definition of COLOR_RED (for example). Other than that, making the changes was very little work.
    – gnasher729
    Aug 27 at 17:29
  • 1
    One codebase I worked with had an image file named "bluetriangle.png"; viewing it revealed a yellow circle.
    – IMSoP
    Aug 27 at 21:43
8

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.

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.

7
  • 5
    I think you should emphasise "if it's only used once". I was about to downvote when I read only the first paragraph of your answer.
    – Bergi
    Aug 24 at 20:12
  • 2
    @Bergi: What matters is not how many times the color is used, but rather whether there are multiple uses that are more likely to be changed together than to be changed separately. If there are five objects that all happen to be the same color, but for different reasons, and which might need to be changed for different reasons, trying to use shared identifiers will increase the difficulty of making changes to some but not all of them.
    – supercat
    Aug 24 at 22:00
  • 1
    @supercat Such a coincidence is rather unlikely :-) And still I would always given them an identifier that names the color (TURQUOISE, DARK_BLUE etc) so that one can quickly find all the five objects that use this color, then decide whether setColor(DARK_BLUE) should be changed to setColor(LIGHT_BLUE) or whether the rgb value of DARK_BLUE itself should be made a bit lighter. Of course another level of indirection, where you name the value by purpose (e.g. LABEL_COLOR, BUTTON_COLOR) is good (only) if you have multiple occurrences of a color that should change for the same reason.
    – Bergi
    Aug 24 at 22:39
  • @Bergi: Which coincidence is unlikely--that colors which should be separately changeable (e.g. the color of a stop sign versus the color of a stop light), or that multiple things that use the same color should be changed together (e.g. the color of a red-circle traffic signal, versus the color of a red-arrow traffic signal)?
    – supercat
    Aug 25 at 14:50
  • 1
    If there are several unrelated uses, it's then very important to give them names or otherwise identify that they should not be changed together! It's a frustrating part of maintenance to trawl through code identifying which values match by design and which by coincidence. Aug 26 at 9:17
4

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.

5
  • 2
    Actually, Apple might be unhappy if you make changes to your application on the AppStore with a configuration file that can be changed at any time, so the app that they have reviewed isn't the app that the user sees. Unless you want something to be user configurable, set it at build time.
    – gnasher729
    Aug 24 at 15:55
  • Sorry, I didn't see this tagged as apple, but more general. Apple certainly allows for configuration. Also there are schemes for the user to customize apps color schemes and so forth, if that is desired, and all of these would remove magic numbers from the code.
    – Erik Eidt
    Aug 24 at 18:01
  • 2
    @gnasher729, "Unless you want something to be user configurable, set it at build time." — yes, but that doesn't mean hard-coding magic numbers in your source code. My answer post is really just general purpose recommendation to get the magic numbers out of the code, so graphic designers, if not end-users can tweak things without needing recompliation.
    – Erik Eidt
    Aug 24 at 18:02
  • From experience, I don't let graphic designers anywhere near my source code. Good experiences with them messing up a source code repository so that we were not able to build a three month old version anymore. Or having 1000 tiny icons that each were 200 kilobyte in size.
    – gnasher729
    Aug 27 at 17:32
  • @gnasher729, yep, one more reason to get magic numbers out of programming language source code.
    – Erik Eidt
    Aug 27 at 17:40
1

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.

1
  • 1
    I once was confronted with an API change where vertical and horizontal positions in the constructor for a rectangle had been swapped. Which meant everything compiled but was in a completely wrong place. That's where the thought of murder enters your mind. (I love named parameters, where such nonsense makes the compilation break, so you know how to fix it).
    – gnasher729
    Aug 27 at 17:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.