A while ago I came across a particulary irritating piece of code in a somewhat harmlessly looking mathematical vector class. C++’s rare feature of operator overloading makes it a good fit for multi-dimensional calculations, so vector classes are common and I had already seen quite a few of them in my career. It looked something like this:
template <typename T>
class vec2
{
public:
/* A few member functions.. */
bool operator==(vec2 const& rhs) const;
T x;
T y;
};
Not many surprises here, except that maybe the operator==() should be a free-function instead. Whether the data members of the class are an array or named individually is often a point of difference between vector implementations. Both certainly have their merits. But I digress…
What really threw me off was the implementation of the operator==(). How would you implement it? Intuitively, I would have expected pretty much this code:
template <typename T>
bool vec2<T>::operator==(vec2 const& rhs) const
{
return x==rhs.x && y==rhs.y;
}
However, what I found instead was this:
template <typename T>
bool vec2<T>::operator==(vec2 const& rhs) const
{
if (x!=rhs.x || y!=rhs.y)
{
return false;
}
return true;
}
What is wrong with this code?
Think about that for a moment! Can you swiftly verify whether this boolean logic is correct? You actually need to apply De Morgan’s laws to get to the expression from the first implementation!
This code was not technically wrong. In fact, for all its technical purposes, it was working fine. And it seems functionally identical to the first version! Still, I think it is wrong on at least two levels.
Different relations
Firstly, it bases its equality on the inequality of its contained type, T. I found this quite surprising, so this already violated the POLA for me. I immediately asked myself: Why did the author choose to implement this based on operator!=(), and not on operator==()? After all, supplying equality for relations is common in templated C++, while inequality is inferred. In a way, this is more intuitive. Inequality already has the negation in its name, while equality is something “original”! Not only that, but why base the equality on a different relation of the contained type instead of the same? This can actually be a problem when the vector is instantiated on a type that supplies operator==(), but not operator!=() – thought that would be equally surprising. It turned out that the vector was only used on built-in types, so those particular concerns were futile. At least, until it is later used with a custom type.
Too many negations
Secondly, there’s the case of immediately returning a boolean after a condition. This alone is often considered a code-smell. It could be argued that this is more readable, but I don’t want to argue in favor of pure brevity. I want to argue in favor of clarity! In this case, that construct is basically used to negate the boolean expression, further obscuring the result of the whole function.
So basically, the function does a double negation (not un-equal) to express a positive concept (equal). And negations are a big source of errors and often lead to confusion.
Conclusion
You need to make sure to make the code as simple and clear as possible and avoids any surprises, especially when dealing with the relatively unconstrained context of C++ templates. In other words, you need to make sure to meet the expectations of the naive reader as well as possible!
Schneide Blog 