tags:

views:

166

answers:

7

Writing an operator< () for a struct appears to be clearer than writing the classical trivalue compare.

for example, to sort the following

struct S {
    int val;
};

you can write an operator< ()

bool operator< ( const S &l, const S &r ) {
     return l.val < r.val;
}

or, a trivalue function (usually in the following fashion )

int compare( const S &l, const S &r ) {
    if( r.val > l.val ) return 1;
    if( r.val < l.val ) return -1;
    return 0;
}

The former is clearer, therefore you can say there's better code quality. The latter forces you to think of 3 cases, which complicates code.

But this thought is a bit deceiving in more complex structures:

struct S {
    int x;
    int y;
};

the following is clear, and begginners tend to write it like so

bool operator< ( const S &l, const S &r ) {
     if( l.x < r.x ) return true;
     if( l.y < r.y ) return true;
     return false;
}

but it's wrong ! You can't sort correctly with this !

And it takes some time to think that you actually have to write it like so

bool operator< ( const S &l, const S &r ) {
     if( l.x < r.x ) return true;
     if( l.x > r.x ) return false;
     if( l.y < r.y ) return true;
     if( l.y > r.y ) return false;
     return false;
}

for it to work correctly.

Can you, and do you write this sort of compare function in a nicer/clearer manner ? The old trivalue compare function at least 'forced' you into thinking about >, <, and == cases.

+4  A: 

The thing is that you are fine with just declaring one trivalue compare function if you autogenerate all operators using: http://en.wikipedia.org/wiki/Barton%E2%80%93Nackman_trick

Let_Me_Be
+1, Very reminiscent of Haskell. This would also have been my advice.
Konrad Rudolph
You don't need a trivalue comparison function to generate all the operators, it just requires combining two comparisons.
Mark B
@Mark You need three values to generate `<,>,<=,>=,==,!=`
Let_Me_Be
@Let_Me_Be Sorry, I was trying to say if you have `<` or `>` you can generate all six.
Mark B
@Let_Me_Be: it's called `boost::less_than_comparable` from Boost.Operators. Very handy, but doesn't solve the issue of writing a proper `operator<` in the first place.
Matthieu M.
@Matthieu Well with the original Barton-Nackman trick you don't write any operators. Just a trivalue compare method.
Let_Me_Be
@Let_Me_Be: I don't understand what you mean. Whether you write `operator<` or `cmp` is still writing a method that you need to get right, and both are of equal complexity...
Matthieu M.
@Matthieu Ugh, this is the second time in this thread of comments. You need three values to generate all operators, therefore implementing just `operator<` is not enough! That's the whole point of my answer. You are fine with just one method that returns 3 values. From that you can auto-generate all the comparison/equality operators using the Barton-Nackman trick.
Let_Me_Be
Matthieu M.
@Matthieu OK, yes, if you are fine with more then one comparison being done, then yes, you are fine with just `operator<`.
Let_Me_Be
@Let_Me_Be: note that the two comparisons only occurs with `==` and `!=` and that writing `cmp` might be a pessimization if you actually need two comparisons to define it, that will then always be used :) Also, because of this pessimization, Boost.Operators normally require `==` to be defined separately and does not infer it from `<`, it does infer `!=` from `==` though.
Matthieu M.
+4  A: 

I like to do it like this:

bool operator< ( const S &l, const S &r )
{
    if( l.x != r.x ) return l.x < r.x;
    else return l.y < r.y;
}

EDIT: note that this is also one useful feature of std::pair too - it defines this already so you can't make the mistake.

Mark B
+4  A: 

In the int case you can simply write:

return l.x < r.x || (l.x == r.x && l.y < r.y);

Only of you are talking about a type that doesn't have == with the correct behaviour do you need to use something more complex, even then it's not too bad.

return l.x < r.x || (!(r.x < l.x) && l.y < r.y);

Extending to more members:

return l.x < r.x ||
      !(r.x < l.x) && (l.y < r.y ||
      !(r.y < l.y) && (l.z < r.z ||
      /* ... */
      ) /* lisp-like sequence of ) */ );

If you can arrange your members to be in an array or other container you can use std::lexicographical_compare.

Charles Bailey
hmm, I like the lexicographical_compare idea !!
Grim Fandango
A: 

One thing I did once which seemed a useful idiom was to write a compare function which returns a boolean given takes two arguments plus a boolean called "bias". The function returns true if the first argument is greater, or if "bias" is set and the two arguments are equal. Thus, a compare whose result depended upon two sub-comparisons would be:

  if (compare(a.part1,b.part1,compare(a.part2,b.part2,bias)))
    ...

Note that this will do 'extra' comparisons, since part2's will be compared even if the part1 compare would be sufficient to yield an answer, but if avoids redundant tests for equality.

Otherwise, using tri-value logic, one could do something like:

  int result;

  if ((result = compare(a.part1,b.part1)) != 0) return result;
  if ((result = compare(a.part2,b.part2)) != 0) return result;

The latter form avoids unnecessary comparisons, and is easily extensible to any number of fields.

supercat
+4  A: 

If I don't care about performance or compiler spew, I tend to use this:

return make_tuple(l.x, l.y, ...) < make_tuple(r.x, r.y, ...);

And for a slightly less expensive in terms of copies version:

return tie(cref(l.x), cref(l.y), ...) < tie(cref(r.x), cref(r.y), ...);

Incidentally, the second version also works with lvalues.

MSN
Interesting idea. But wouldn't `return tie(l.x, l.y, ...) < tie(r.x, r.y, ...);` avoid a copy? (I assume this creates tuples of references).
UncleBens
@UncleBens... holy crap, I didn't think of that! I guess my initial disclaimer was even worse than I thought :) Although it doesn't work if l or r are or contain const values.
MSN
Seems to compile for me (GCC 4.3): it creates const references.
UncleBens
+1 for UncleBens comment :) Do include it in your answer, it certainly is the best way!
Matthieu M.
@UncleBens, on VC8, it barfs saying none of the two overloads could convert all the argument types. :(
MSN
@MSN: I don't have VC8 handy, have you tested with `make_tuple(cref(l.x), cref(l.y), ...)` ?
Matthieu M.
I actually did this with a member function call, not a member. `tie(l.x)<tie(r.x)` works fine, but `tie(l.x())<tie(r.x())` does not. @Matthieu, yes, `adding std::ref(...)` makes `make_tuple` work without making copies.
MSN
@MSN: I would really use `cref` here, `l` and `r` are probably `const` qualified (or should) and thus you'll need the const-ness.
Matthieu M.
@Matthieu, done and done.
MSN
@MSN: I wish I could upvote once more :-(
Matthieu M.
@Matthieu, @UncleBens, turns out I forgot to `#include <functional>`, so now the original version works even with lvalues :)
MSN
A: 

For the first tri-value compare() function

int compare( const S &l, const S &r ) {
    return r.val - l.val;
}

For the later

bool operator< ( const S &l, const S &r ) {
     return (l.x < r.x) || ((l.x == r.x) && (l.y < r.y));
}
ArunSaha
The tri-value version cannot be used in the general case, because the result may under- or overflow.
UncleBens
A: 

This is no clearer or shorter than your last example, but it does have the advantage of not requiring anything other than operator< on the members.

bool operator< ( const S &l, const S &r ) { 
     if( l.x < r.x ) return true; 
     if( r.x < l.x ) return false; 
     if( l.y < r.y ) return true; 
     if( r.y < l.y ) return false; 
     return false; 
} 

The last case can always be simplified, unfortunately the prior cases must always be the longer form.

bool operator< ( const S &l, const S &r ) { 
     if( l.x < r.x ) return true; 
     if( r.x < l.x ) return false; 
     return  l.y < r.y; 
} 
Mark Ransom