10

I have a bit of a theoretical question, however it is a problem I sometimes face when designing classes and I see it done differently when reading others code. Which of the following would be better and why:

example 1:

class Color
{
public:
  Color(float, float, float);
  ~Color();

  friend bool operator==(Color& lhs, Color& rhs);
  void multiply(Color);
  // ...
  float get_r();
  float get_g();
  float get_b();

private:
  float color_values[3];
}

example 2:

class Color
{
public:
  // as above

private:
  float r;
  float g;
  float b;
}

Is there a general rule one should follow in cases like this or is it just up to a programmer and what seems to make more sense?

jaho
  • 4,852
  • 6
  • 40
  • 66
  • BTW do you have any practical reason for why to keep the fields private here? – Kos Jul 04 '12 at 16:10
  • @Kos, given the example, I could easily see using accessors and mutators to restrict the input, although each field should probably be `byte` instead of `float`. Additionally, string values such as `#ABCDEF` and `#ABC` might be used. – zzzzBov Jul 04 '12 at 20:32

7 Answers7

10

Both!

Use this:

class Color {

    // ...

private:

    union {
       struct {
           float r, g, b;
       };
       float c[3];
    };

};

Then c[0] will be equivalent to r, et cetera.

Kos
  • 70,399
  • 25
  • 169
  • 233
  • 1
    Best actual answer, get the best of both worlds ! – Julien Lebot Jul 04 '12 at 15:57
  • 1
    +1 for cleverness. I hadn't thought of this. (Regarding the language standard legalistically, I am not 100 percent certain that this must always work, but even if not legalistically, still practically it will work. It is hard to imagine compiler that were likely to break your answer.) – thb Jul 04 '12 at 15:57
  • @thb It is well defined if `sizeof(the_anonymous_struct_with_the_three_floats) == 3*sizeof(float)`, which is not an insane assumption. I guess that if you're paranoid, you can `static_assert` it. – R. Martinho Fernandes Jul 04 '12 at 16:05
  • Undefined behavior and gratuitois cleverness. – John Dibling Jul 04 '12 at 16:37
  • True, it's undefined; the standard could be just a little more elaborate when defining "common initial subsequence". – Kos Jul 04 '12 at 16:43
4

It depends, do you intend to iterate over the whole array ?

In that case, I think solution 1 is more appropriate. It is very useful to have an array like that when you have functions that operate in a loop on the data

e.g.

void BumpColors(float idx)
{
    for (int i = 0; i < 3; ++i)
        color_values[i] += idx;
}

vs

void BumpColors(float idx)
{
    color_values[0] += idx;
    color_values[1] += idx;
    color_values[2] += idx;
}

Of course this is trivial, and I think it really is a matter of preference. In some rare occasion you might have APIs that take a pointer to the data though, and while you can do

awesomeAPI((float*)&r);

I would much prefer doing

awesomeAPI((float*)&color_values[0]);

because the array will guarantee its contiguity whereas you can mess up with the contiguity by adding by mistake another member variable that is not related after float r.


Performance wise there would be no difference.

Julien Lebot
  • 3,092
  • 20
  • 32
  • The iteration argument is IMHO the key, but I think in the case of colors, it argues the other way. I can't think of a case where it would make sense to increase each color by the same value. – James Kanze Jul 04 '12 at 15:56
  • We can't make assumption as to what the user is going to do with the color; for all we know they could use the Color class to store spherically-encoded vertex normals. – Julien Lebot Jul 04 '12 at 15:59
3

I'd say the second one is the best one.

First, the data your variables contain isn't supposed (physically) to be in an array. If you had for example a class with 3 students, not more, not less, you'd put them in an array, cause they are an array of students, but here, it's just colors.

Second, Someone that reads your code also can understand in the second case really fast what your variables contain (r is red, etc). It isn't the case with an array.

Third, you'll have less bugs, you won't have to remember "oh, in my array, red is 0, g is 1, b is 2", and you won't replace by mistake 

return color_values[0]

by

return color_values[1]

in your code.

Tuxer
  • 753
  • 1
  • 6
  • 20
  • That is true, but at the end of the day it depends what (s)he is going to use the colours for. If she is going to convert from RGB to HSL, which is basically a vector operation, the array might be more convenient. Also, there's the possibility to index the array with an enum, like 'color_values[red]' – deStrangis Jul 04 '12 at 15:52
  • yeah, I thought about the enum, but it's a bit overkill for such an easy class :D – Tuxer Jul 04 '12 at 18:13
  • +1. Meaning is totally obscured by the generic array referencing. Further, an array conveys the wrong sense of the data. r,g,b (badly named of course) are different things. They are not interchangeable in iterative processing; there is no way to properly set or get values without knowing context. Therefore we cannot use the benefit of the array structure. – radarbob Jul 17 '12 at 22:24
2

I think that you are right: "It just up to a programmer and what seems to make more sense." If this were my program, I would choose one form or the other without worrying too much about it, then write some other parts of the program, then revisit the matter later.

One of the benefits of class-oriented design is that it makes internal implementation details of this kind private, which makes it convenient to alter them later.

I think that your question does matter, only I doubt that one can answer it well until one has written more code. In the abstract, there are only three elements, and the three have names -- red, green and blue -- so I think that you could go either way with this. If forced to choose, I choose example 2.

thb
  • 13,796
  • 3
  • 40
  • 68
1

Is there a general rule one should follow in cases like this or is it just up to a programmer and what seems to make more sense?

It's definitely up to the programmer and whatever makes more sense.

In your case, the second option seems more appropriate. After all, logically thinking, your member isn't an array of values, but values for r, g and b.

Luchian Grigore
  • 253,575
  • 64
  • 457
  • 625
0

Advantages of using an array:

  • Maintainability: You can use the values in the array to loop
  • Maintainability: When a value should be added (like yellow?) than you don't have to change a lot of code.

Disadvantage:

  • Readability: The 'values' have more clearer names (namely r, g, b in this case).

In your case probably the r, g, b variables are best, since it's unlikely a color is added and a loop over 3 elements has probably a less high importance than readability.

Michel Keijzers
  • 15,025
  • 28
  • 93
  • 119
0

Sometimes a programmer will use an array ( or data structure ) in order to save the data faster to disk (or memory) using 1 write operation. This is especially useful if you are reading and writing a lot of data.

Michael C.
  • 1