Is this overly clever or unsafe?

Question

I was working on some code recently and decided to work on my operator overloading in c++, because I've never really implemented it before. So I overloaded the comparison operators for my matrix class using a compare function that returned 0 if LHS was less than RHS, 1 if LHS was greater than RHS and 2 if they were equal. Then I exploited the properties of logical not in c++ on integers, to get all of my compares in one line:

inline bool Matrix::operator<(Matrix &RHS){
  return ! (compare(*this,RHS));
}
inline bool Matrix::operator>(Matrix &RHS){
  return ! (compare((*this),RHS)-1);
}
inline bool Matrix::operator>=(Matrix &RHS){
  return compare((*this),RHS);
}
inline bool Matrix::operator<=(Matrix &RHS){
  return compare((*this),RHS)-1;
}
inline bool Matrix::operator!=(Matrix &RHS){
  return compare((*this),RHS)-2;
}
inline bool Matrix::operator==(Matrix &RHS){
  return !(compare((*this),RHS)-2);
}

Obviously I should be passing RHS as a const, I'm just probably not going to use this matrix class again and I didn't feel like writing another function that wasn't a reference to get the array index values solely for the comparator operation.

As per suggestion here is the code if Compare returns -1 for less, 0 for equal and 1 for positive.

inline bool Matrix::operator<(Matrix &RHS){
  return ! (compare(*this,RHS)+1);
}
inline bool Matrix::operator>(Matrix &RHS){
  return ! (compare((*this),RHS)-1);
}
inline bool Matrix::operator>=(Matrix &RHS){
  return compare((*this),RHS)+1;
}
inline bool Matrix::operator<=(Matrix &RHS){
  return compare((*this),RHS)-1;
}
inline bool Matrix::operator!=(Matrix &RHS){
  return compare((*this),RHS);
}
inline bool Matrix::operator==(Matrix &RHS){
  return !(compare((*this),RHS));
}

I don't know that this really increases the readability though.

Answer 1

Yes, it's overly clever - I read that code and have to think about why you're subtracting two from a function called compare. Don't make me think.

If you're being clever to make your code fit on one line then you've got muddled priorities. You should use as many lines as needed to make your code as clear as possible.

Programs must be written for people to read, and only incidentally for machines to execute. (Abelson & Sussman, Structure and Interpretation of Computer Programs)

Answer 2

Yes, this is too complex.

compare should return 0 for equal values, positive if this is bigger and negative if this is smaller.

It would be much simpler and would be of even performance.

If I would given this code for review, I would mark this as something that should be fixed.

Answer 3

Clever?

warning: please see comments

I think it is unefficient for!=to callcompare (which checks all values of both matrices in order to find out which one is bigger) because ifm1[0][0]!=m2[0][0]then!=could already returnfalse. So i think it's a nice idea to simplify writing these operators and if performance doesn't matter at all, it can be considered clever. But if performance does matter, it is not clever.

Safe?

I also think it is safe because it produces the right results.

Answer 4

As far as I can see it's safe, but it does take looking twice for everybody reading the code. Why would you want to do this?

Anyway, for comparison, all you ever need is < and either == or !=, the rest is canonical and I write it mostly by muscle memory. Also, binary operators treating their operands equally (they leave them alone) should IMO be implemented as non-members. Given this, plus using the sane comparison function (-1, 0, +1) and adding the necessary const, I come to this:

// doing real work
inline bool operator<(const Matrix& l, const Matrix &r)
{
  return -1 == compare(l,r);
}
inline bool operator==(const Matrix& l, const Matrix &r)
{
  return 0 == compare(l,r);
}
// canonical
inline bool operator> (const Matrix& l, const Matrix &r) {return r < l;}
inline bool operator>=(const Matrix& l, const Matrix &r) {return !(l < r);}
inline bool operator<=(const Matrix& l, const Matrix &r) {return !(r < l);}
inline bool operator!=(const Matrix& l, const Matrix &r) {return !(l == r);}

The comparisons might not be as clever as yours, but everyone who's ever seen strcmp() knows immediately what they do. Note that I even added 0 != compare(...), which is completely unnecessary - for the compiler. For humans IMO it makes it more clear what's going on than the implicit cast to bool. Plus it emphasizes the symmetry to operator<'s implementation.

Answer 5

Comparing the output of compare to 0 using any of the six comparison operators will yield the correct result for the corresponding overloaded operator. That way your code will be very readable and it will be instantly obvious that it's correct (if compare is correctly implemented).

inline bool Matrix::operator < (const Matrix &RHS){
  return compare(*this, RHS) < 0;
}
inline bool Matrix::operator > (const Matrix &RHS){
  return compare(*this, RHS) > 0;
}
inline bool Matrix::operator >= (const Matrix &RHS){
  return compare(*this, RHS) >= 0;
}
inline bool Matrix::operator <= (const Matrix &RHS){
  return compare(*this, RHS) <= 0;
}
inline bool Matrix::operator != (const Matrix &RHS){
  return compare(*this, RHS) != 0;
}
inline bool Matrix::operator == (const Matrix &RHS){
  return compare(*this, RHS) == 0;
}

Answer 6

As mentioned before, I think the standard way would be to return < 0 when LHS < RHS, > 0 for LHS > RHS, and 0 for equality.

But may I ask another question? Why use operator overloading here at all? The idea behind operator overloading is (or should be) to be able to use objects in an intuive way. But as far as I know, there is no standard definition for comparing matrices other than for (in)equality. At least I know of none. So what shall I think of when I read M1 < M2?

Just let me guess: operator<() and operator>() were just added for completeness, but will never actually be used in real world code - right? If so, don't implement them.

Answer 7

If you worry whether something is overly clever ... it probably is :-)

Answer 8

I'll give you my own way:

#include <boost/operators.hpp>

class Matrix: boost::equality_comparable<Matrix
            , boost::less_than_comparable<Matrix
              > >
{
}; // class Matrix

bool operator==(const Matrix&, const Matrix&);

bool operator<(const Matrix&, const Matrix&);

I use less lines than you do without any clever tricks. As for Boost ? Well, it's pretty standard by now and it's documented online. You can always add some comments:

// boost::equality_comparable: automatically generate != from ==
// boost::less_than_comparable: automatically generate >, <=, >= from <
// search for Boost.Operators on the web to get more information

A last word: I don't know about the particular application you are writing, but using matrices I always found it a good idea to have a Matrix base class and some TMatrix subclass (template, as the T indicates) with dimensions known at compile time. You can then provide operators on TMatrix which can only deal with matrices of similar dimensions, since anything else is heresy, and thus have compile-time diagnosis.