0

I need to write a Color class for a small application with RGBA values. I was thinking of having something like:

class Color
{
public:
    Color(int red, int green, int blue);
    // stuff

private:
    int m_red;
    int m_green;
    int m_blue;
    int m_alpha;
};

I would like to add the possibility of using a couple of predefined colors. I have seen an example of someone using static constants to do this, but I have always heard that they are not good practice because they pollute the app and can cause problems in tests. I was wondering what in your opinion would be the best way to do this, and why.

Regards

BobMorane
  • 3,870
  • 3
  • 20
  • 42
  • Can you specify how they would pollute an app or what problems they would cause in tests? Realistically I don't see any issue with using static constants for this. – dempzorz May 23 '17 at 00:40

2 Answers2

3

Although I think that think this question will be closed as primarily opinion based, I will offer my opinion.

The main reason that I know of to avoid static constants is that you can run into linker errors if you define them inline and need to take them by reference. See this. Since this isn't an integral type, you would either have to make the class constexpr and define static constants inline(outside of the class) or define them outside of the class in an implementation file. Either way requires an implementation file (the inline constexpr constants version is the one that will cause linker errors).

I would prefer to make such a simple class constexpr and define the predefined colors as static constexpr functions:

struct Color
{
    static constexpr Color red() { return Color(255, 0, 0); }
    static constexpr Color green() { return Color(0, 255, 0); }
    static constexpr Color blue() { return Color(0, 0, 255); }
    static constexpr Color black() { return Color(0, 0, 0); }
    static constexpr Color white() { return Color(255, 255, 255); }

    constexpr Color(uint8_t r, uint8_t g, uint8_t b, uint8_t a = 255)
        : r(r), g(g), b(b), a(a)
    {}

     uint8_t r;
     uint8_t g;
     uint8_t b;
     uint8_t a;
};

void do_something_with_color(const Color& color)
{
    printf("worked\n");
}

int main()
{
    do_something_with_color(Color::red());
    return 0;
}

As a side note, this is one of those times when you really are dealing with a struct. The public interface for the type is its components, so they should be public. This function-based approach has no performance penalty, avoids the possible linker errors, and is more flexible and cleaner in my opinion.

rationalcoder
  • 1,587
  • 1
  • 15
  • 29
  • You only get linker errors if you define it inline and then the constant is ODR used. It's not particularly good practice to define things inline anyway generally speaking so this shouldn't really be an issue. Your approach is fine too. It's 6 of one and half a dozen of the other.The only potential issue with your approach is that if there are functions that take color by reference, and a user only cares about the output of said function, with your approach you have to construct a new color each time. With static variables you don't. – Nir Friedman May 23 '17 at 03:07
  • @NirFriedman I'm not sure what you mean about the constructing a new color each time thing. – rationalcoder May 23 '17 at 03:44
  • One would hope that `constexpr` would inform the compiler that it only need evaluate the function once. – Rook May 23 '17 at 05:50
  • @Rook It does. I'm not sure what he is on about. – rationalcoder May 23 '17 at 06:02
  • @Rook If there's a function `foo` that takes a color by const reference, and there ends up being `foo(Color::blue())` in multiple different points in the code, each one of those calls is constructing a new `Color` object. On the other hand, I have no idea what you guys are talking about with "informing the compiler that it need evaluate the function once". – Nir Friedman May 23 '17 at 13:36
  • @NirFriedman A `constexpr` expression with constant arguments (eg. no arguments, in this case) _may_ be evaluated at compile time. Turns out that this isn't guaranteed. I don't know what happens when it is invoked at runtime, though the compiler does know that the value cannot change at runtime. – Rook May 23 '17 at 13:59
  • Presumably you could do `static const Color& red() { static const Color r = Color(255, 0, 0, 1); return r; }` as initialisation of static locals is guaranteed to be done only once, on first invocation, right? – Rook May 23 '17 at 14:02
  • @Rook I'm well aware of that, but none of that matters if you are passing it into a non constexpr function that e.g. does not get inlined. It still always needs a fresh copy of the object, otherwise the compiler might be changing behavior. I'm pretty sure that in a large program if you call `red()` multiple times you should expect a fresh construction for each call, in general. – Nir Friedman May 23 '17 at 14:54
  • @Rook As for static local, yes this will work though you will be introducing a branch at every point it is used, plus more intensive atomic instructions the first time it is used. – Nir Friedman May 23 '17 at 14:55
  • @NirFriedman good point. Clearly it is time to suggest a trampoline... ;-) – Rook May 23 '17 at 14:58
  • @NirFriedman This is a non issue. That is the reason it avoids linker errors. This way, the color class doesn't need a cpp file. Also, constructing a new Color here is a simple 4 byte assignment. If anything, taking a reference to a copy on the stack could end up faster. Even then, taking a color by const reference would be rare; taking it by value would be much more common, as it is 4 bytes, in which case copying from a static reference would be no faster(probably slower) than a 1 or 2 4 byte assignments. If you were still worried, you could create a temporary and reuse it. – rationalcoder May 23 '17 at 17:29
1

With namespaces you can hide the instances under a namespace, but normally, they are declared as static fields

class Color {
public:
    static const int MAX = 0xffff;
    Color(int red, int green, int blue, int alpha = MAX);
    // stuff

    static const Color red;
    static const Color green;
    static const Color blue;
    static const Color white;
    static const Color black;
    // ...

private:
    int m_red;
    int m_green;
    int m_blue;
    int m_alpha;
};

const Color Color::red(MAX, 0, 0);
const Color Color::green(0, MAX, 0);
const Color Color::blue(0, 0, MAX);
const Color Color::white(MAX, MAX, MAX);
const Color Color::black(0, 0, 0);
Luis Colorado
  • 10,974
  • 1
  • 16
  • 31
  • As it stands, unless the intent was to waste space and provide an easy way to construct invalid colors(negative and past "MAX" components), the code needs to be updated. If you really intended to provide RGBA16, you should at least change the components to uint16_t. Also, the point of this was to provide a better solution than static constants or explain why they are the best option. They are clearly not the best option, here. This solution requires an implementation file. – rationalcoder May 24 '17 at 13:49
  • The example was not directed but to give an example. The code is only an example, and answers the question formulated. I don't intend to provide RGBA16, nor any existent API in use, but just to give an example of static object instantiation with read only attributes, to avoid for client software to modify it. The code compiles correctly and is compliant with the original Stroustrup specification. But good intent!!! :) – Luis Colorado May 25 '17 at 19:13
  • 1. I never said it didn't compile. 2. Your definition of MAX implies RGBA16. 3. The OP's last paragraph(the link to SFML's Color class) implies that he is aware of the possibility of using statics like this, but is concerned that they are bad practice. You have basically just shown him what he has already seen, without an explanation as to why he shouldn't opt for a different solution. As I am aware, my solution is strictly better than SFML's, and, since this is the answer you gave, you should justify using it over mine. – rationalcoder May 25 '17 at 20:06
  • @rationalcoder, not, it doesn't implies that. I don't know about RGBA16, just taken the example posted by the applicant. Probably he's aware of something, but I am not. Don't fire to me, I tried just to help, and my example is good to illustrate the thing. If your solution is better or worse, I don't mind about. I don't understand what exactly are you going to state, if your solution is better than mine is not going to be decided in this discussion. Subject closed. – Luis Colorado May 26 '17 at 05:24