-1
\$\begingroup\$

I have a C++ function that is called frequently within a loop. It compares the variables v_1, v_2 and v_3 with type double of two objects a and b of a class MyClass in the following way:

// a, b are objects of a class MyClass
// v_1, v_2 and v_3 are class variables of type double

bool compare(MyClass const *a, MyClass const *b) const {

    if(a->v_1 != b->v_1) {

        return(a->v_1 > b->v_1);   // Usual case
    }

    else if(a->v_2 != b->v_2) {

        return (a->v_2 < b->v_2);  // Happens quite some times
    }

    else {

        return (a->v_3 < b->v_3);  // Unusual case
    }
}

I suspect that there is a better/easier way to implement this functionality. Do you have an idea on how to improve both style and, if possible, also efficiency of this function?

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Is it intended that the first comparison uses > while the other 2 use <? \$\endgroup\$
    – nwp
    Nov 23 '17 at 15:48
  • 1
    \$\begingroup\$ Can you explain at a high level what compare does? It doesn't seem to be simply comparing MyClasss but do something more complicated instead. \$\endgroup\$
    – nwp
    Nov 23 '17 at 16:26
  • \$\begingroup\$ @nwp Regarding your first comment, yes that's intended. \$\endgroup\$
    – mindm49907
    Nov 23 '17 at 16:50
  • \$\begingroup\$ @nwp Regarding your second comment, the function is actually a comparison that is used for picking objects of Myclass from a priority queue based on the comparisons shown there. \$\endgroup\$
    – mindm49907
    Nov 23 '17 at 16:53
  • 1
    \$\begingroup\$ There really isn't enough context to properly review this - for instance, you've omitted the definition of the (vaguely-named) MyClass. Depending on the types of its members, you might be able to write a clearer one-liner such as return std::tie(b->v1, a->v2, a->v3) < std::tie(a->v1, b->v2, b->v3);. \$\endgroup\$ Nov 23 '17 at 17:11
2
\$\begingroup\$

Naming

MyClass is a poor name for a type - type names should convey what they are used for.

Encapsulation

The compare() appears to be a member function (as it has a const decoration). If it's a member of MyClass, then it should either be static instead, or it should use the this argument it implicitly receives. And it could be named operator< instead, to increase its utility:

bool MyClass::operator<(const MyClass& other) const

If it's not a member function, then the const is a syntax error. Not only that, but it implies that the members of MyClass are public, which seems wrong for a "class" (as opposed to a struct).

Implementation

Comparing pairs of values in turn until a pair is unequal is a common pattern, and the usual means to make it short and clear is to make use of the built-in less-than operator of std::tuple, something like this:

// Note that `v1` sorts in descending order, so is swapped in the tuples
return std::make_tuple(other.v1,       v2,       v3)
     < std::make_tuple(      v1, other.v2, other.v3);

If you don't want to copy the members (perhaps they are of class type), you could use std::tie() instead of std::make_tuple(), to compare through references instead.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your suggestions! The original code has more useful variable names, of course. Regarding encapsulation and implementation, I will try the things you suggested and post again what I've decided to use. As you might notice, I'm a beginner at C++, so thanks for pointing it out the std::tie function! \$\endgroup\$
    – mindm49907
    Nov 24 '17 at 9:32

Not the answer you're looking for? Browse other questions tagged or ask your own question.