Nasty trap in constructor of Colour!


#1

I spent far too long on this one…

As I mentioned before, constructors are bad in every language.

I had code like this:

uint8 r, g, b; Colour c(r, g, b);

which was working fine. Then I needed to get float colors in a few cases… and looking at Colour, how nice, it supports floats! So I changed the code to

float r, g, b; Colour c(r, g, b);

which looks fine… right?

Unfortunately, all of the Colour constructors that take floats are four argument constructors… so of course C++ was secretly casting these floats to an int, the int “0” in fact, and then making a Colour(0, 0, 0).

So I couldn’t work out why some of the Colours kept becoming black for the longest time…

Simplest solution to save the next guy - add a three-argument float constructor for Colour. It’s consistent with the rest of the interface, and convenient.


#2

Heh, and it turns out that that constructor is an HSV constructor anyway.

Moral is again to avoid the constructor for the factory function if possible (and Jules has considerately provided exactly the functions needed).


#3

Ah, sorry to catch you out there! It’s an old, old class, and those constructors have been around for a long time, so although I added static methods to replace them, I’m a bit reluctant to remove them because there are bound to be a lot of people using them.


#4

Perhaps things that are considered deprecated could eventually be marked as such in the doxygen api docs? It is sometimes a bit difficult to determine what is the most up-to-date method of doing things.


#5

Well, the other constructors aren’t really deprecated, and there’s nothing technically wrong with them, it just makes your code more readable if you use the static methods instead. I will probably remove/deprecate them eventually to prevent confusion!


#6

Oh, it’s my fault for not looking more closely at the code! I mean, even if I’d gotten the “correct” constructor, I’d still have been passing in RGB values to an HSV constructor.

Still, you can fix such bugs on both sides - the producer (Juce) and the consumer (me).

The first time I ran into this issue, I had a bit of a discussion with the project manager. His claim was that once you have committed the sin of having confusing constructor overloads, you need to repent entirely and replace everything by static factory methods, simply so there’s a unified way of doing everything and future developers don’t ask, “Why are you using a constructor here and a factory method there?”

I had to concede that he was right.

Method overloading, function overloading and constructor overloading are all dangerous as they can lead you to bugs where you can look at the code over and over and never see your mistake.

In fact, that place had an even more dramatic rule - no method or function overloading at all except for special cases (which were, admittedly, quite reasonable). I chafed at that initially, but they eventually wrote up the explanation, and I had to concede that they were right.

Remember, deprecation doesn’t break anything, it’s just a reminder to someone writing a new project what not to do. You can deprecate things and never remove them, look at Java!