4

At work I recently wrote a less than operator for a class implemented from a published specification and which has many properties, six of which are used to uniquely identify an instance of the class. (For the sake of this question, we'll call these properties a-f.) Also, these six properties are of six different types. I defined the operator as such:

bool operator<(const Class& lhs, const Class& rhs)
{
    bool retval = (&lhs != &rhs);
    if (retval == true)
    {
        if (lhs.a == rhs.a)
        {
            if (lhs.b == rhs.b)
            {
                if (lhs.c == rhs.c)
                {
                    if (lhs.d == rhs.d)
                    {
                        if (lhs.e == rhs.e)
                        {
                            retval = (lhs.f < rhs.f);
                        } else {
                            retval = (lhs.e < rhs.e);
                        }
                    } else {
                        retval = (lhs.d < rhs.d);
                    }
                } else {
                    retval = (lhs.c < rhs.c);
                }
            } else {
                retval = (lhs.b < rhs.b);
            }
        } else {
            retval = (lhs.a < rhs.a);
        }
    }
    return retval;
}

This, of course, breaks the Linux kernel coding philosophy of, "If you need more than 3 levels of indentation, you're screwed anyway, and should fix your program." So my question is, is there a better way to define this operator to not have so many levels of indentation?

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
LowTechGeek
  • 431
  • 3
  • 12
  • 1
    When there are so many properties and they fall into a pattern like this, it's likely they could have been implemented as an array. Then the nested `if`s would be a loop. – Barmar Jul 26 '15 at 06:36
  • 2
    I also don't think that coding guideline really applies in a simple pattern like this. It's intended to avoid complex nesting of loops and conditionals. Don't think about the rule, think about the **reason** for the rule. – Barmar Jul 26 '15 at 06:38
  • @Barmar Regarding the array, good idea, but unfortunately the properties are all of different types. I edited the question to state that. – LowTechGeek Jul 26 '15 at 06:42
  • Damn, dude! What SIL are you targeting? – user4581301 Jul 26 '15 at 06:58
  • @user4581301 It's an old, proprietary platform. – LowTechGeek Jul 26 '15 at 07:00
  • Ah. Thanks. I see similar requirements in safety-critical software specs and was starting to have flashbacks. – user4581301 Jul 26 '15 at 07:06
  • I rolled back your edits. It's not fair to change the question after it has been well answered. – David Heffernan Jul 26 '15 at 07:12
  • By the way, your function should be `const`. – paddy Jul 26 '15 at 07:19
  • @DavidHeffernan I agree that there are good answers here, but rolling back the edits also removes key additional information regarding the context in which I designed the code. Can good answers and my edits not both exist? – LowTechGeek Jul 26 '15 at 07:22
  • @paddy Actually, it's not a member function. It uses only public accessors. – LowTechGeek Jul 26 '15 at 07:24
  • @Low It's not fair to change the question and make everyone's answer look stupid. You need to think harder before asking. Take more time to make sure the question is complete before posting it. – David Heffernan Jul 26 '15 at 08:04
  • @LowTechGeek I added a solution that fits your criteria and doesn't involve any stack variables either! Give it a try when you get a chance and let me know how it works out. – Francis Cugler Jul 26 '15 at 08:23
  • 2
    @DavidHeffernan Well, I disagree. Clearly marking the additions as edits lets readers know they were added after the original post. I may have never thought to include that information had it not been for the answers posted pre-edit. Sometimes being able to ask a good question is a _process_. That doesn't make those answers wrong or look stupid. For example, I thought Chris' answer involving `std::tie` was great. On the other hand, removing the edits also removes, as stated above, key contextual information, the absence of which may confuse the reader as to why a particular answer was chosen. – LowTechGeek Jul 26 '15 at 08:41
  • @DavidHeffernan They should allow people to read previous history of any question; like a left and right arrow slide with the left most as the original question and the far right being the last edit! With the Original being the first viewed, and a notice that this page has edits being displayed to the reader. This way when trying to answer questions you can see the original post and how it was edited over time! This would let question & answers evolve better by having a displayable history! – Francis Cugler Jul 26 '15 at 10:42
  • @DavidHeffernan (...continued) Also, they can display who it was that edited along with their comments for why they edited it the way they did. – Francis Cugler Jul 26 '15 at 10:46
  • @Francis the edit history is available – David Heffernan Jul 26 '15 at 11:05

5 Answers5

12

You can write this kind of lexicographical comparison like this:

if (lhs.a != rhs.a) return lhs.a < rhs.a;
if (lhs.b != rhs.b) return lhs.b < rhs.b;
if (lhs.c != rhs.c) return lhs.c < rhs.c;
if (lhs.d != rhs.d) return lhs.d < rhs.d;
if (lhs.e != rhs.e) return lhs.e < rhs.e;
return lhs.f < rhs.f;

You can re-write this with a single return like this:

bool result;
if (lhs.a != rhs.a) result = lhs.a < rhs.a;
else if (lhs.b != rhs.b) result = lhs.b < rhs.b;
else if (lhs.c != rhs.c) result = lhs.c < rhs.c;
else if (lhs.d != rhs.d) result = lhs.d < rhs.d;
else if (lhs.e != rhs.e) result = lhs.e < rhs.e;
else result = lhs.f < rhs.f;
return result;
David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
Brian Bi
  • 111,498
  • 10
  • 176
  • 312
  • 1
    Unfortunately, I also have to adhere to a coding standard that dictates using only one return statement. – LowTechGeek Jul 26 '15 at 06:43
  • @LowTechGeek then you can set a variable and `goto` the return statement – phuclv Jul 26 '15 at 06:58
  • 11
    You need a better coding standard. Only one return statement is a stupid rule. – David Heffernan Jul 26 '15 at 07:10
  • @LưuVĩnhPhúc I can't use goto statements either, although I also don't see how that would avoid the issue. Can you post an example as a possible answer? – LowTechGeek Jul 26 '15 at 07:14
  • 1
    @DavidHeffernan I agree, but work pays the bills, so I follow their rules. – LowTechGeek Jul 26 '15 at 07:15
  • 3
    @Brian I hope you don't mind that I added a variant to meet the post-question asking requirement for there to be a single return statement. – David Heffernan Jul 26 '15 at 07:19
  • 1
    *"I agree, but work pays the bills, so I follow their rules."* Well, then, if they don't also enforce the 3-levels-of-indentation rule, just leave your code as it is. – Christian Hackl Jul 26 '15 at 10:03
  • @ChristianHackl Granted, the code as is works and conforms to all of the standards I have to follow. However, it never felt right to me: it seemed as if it could be better, given the indentation guideline. A solution was not readily apparent, and thus I sought the knowledge of the community. I learned some things and became a bit wiser. To me, that's the whole point of a site like this. – LowTechGeek Jul 26 '15 at 21:43
  • [Should a function have only one return statement?](http://stackoverflow.com/q/36707/995714) – phuclv Sep 22 '15 at 09:47
11

You can use std::tie to do lexicographical comparisons:

bool operator<(const Class& lhs, const Class& r) {
  return std::tie(lhs.a, lhs.b, lhs.c, lhs.d, lhs.e) < std::tie(rhs.a, rhs.b, rhs.c, rhs.d, rhs.e);
}
Chris Drew
  • 14,926
  • 3
  • 34
  • 54
2

Since you only set retval 1 time and return it after setting, you could remove it completely and use return instead. That, alongside reordering your logic could look like:

bool operator<(const Class& lhs, const Class& rhs)
{
    if(&lhs == &rhs)
        return false;

    if (lhs.a != rhs.a)
        return (lhs.a < rhs.a);

    if (lhs.b != rhs.b)
        return (lhs.b < rhs.b);

    // And so on...
}
Amit
  • 45,440
  • 9
  • 78
  • 110
2

This is a properly formatted nested ternary statement. This is also a single line of an execution statement. 

bool operator<( const Class& lhs, const Class& rhs ) const {
    return   lhs.a != rhs.a ? lhs.a < rhs.a
           : lhs.b != rhs.b ? lhs.b < rhs.b
           : lhs.c != rhs.c ? lhs.c < rhs.c
           : lhs.d != rhs.d ? lhs.d < rhs.d
           : lhs.e != rhs.e ? lhs.e < rhs.e
           : lhs.f < rhs.f;
}

// The Above Is The Same As:
bool operator<( const class& lhs, const Class&rhs ) const {
    bool result;
    if (lhs.a != rhs.a) result = lhs.a < rhs.a;
    else if (lhs.b != rhs.b) result = lhs.b < rhs.b;
    else if (lhs.c != rhs.c) result = lhs.c < rhs.c;
    else if (lhs.d != rhs.d) result = lhs.d < rhs.d;
    else if (lhs.e != rhs.e) result = lhs.e < rhs.e;
    else result = lhs.f < rhs.f;
    return result;
}
// The Main Difference Is You Are Not Declaring A Stack Variable To The Compiler
// Nor Are You Using If Else Statements, This Is Handled Automatically By The Compiler
// And This Is Usually Done Within The Registers.
Francis Cugler
  • 7,788
  • 2
  • 28
  • 59
  • The OP's method also checks if lhs and rhs are different references. Moreover the fact that your code doesn't declare any variables doesn't necessarily mean that it doesn't use any variables on the stack. The compiler may allocate some memory location for internal use. It's true esp. on many microcontrollers, where there is only 1 register. – phuclv Jul 26 '15 at 09:43
  • @LưuVĩnhPhúc Yes true I do see your point, its been a while since I've messed with asm; however, you are not directly creating a stack variable for the return value, it is returning the comparison between lhs && rhs using registers; it is just done behind the scenes! I'm just not asking the compiler to say hey here is a bool type variable named retval set this amount of memory for this type aside for me on the stack! (...) – Francis Cugler Jul 26 '15 at 10:31
  • (...continued) If you understand the ternary operator and can recognize the pattern similarities between it and the if else if else clause and know how to format it, it is in my opinion cleaner looking code. – Francis Cugler Jul 26 '15 at 10:32
  • 1
    @Francis who says that the local variable will live on the stack. A good compiler will put in in a register. Your code is unreadable. – David Heffernan Jul 26 '15 at 11:04
  • It is still a stack variable since the heap isn't being used. It is very readable return type is bool. When the function is invoked by the caller it will test each condition until either a final true or false is reached then it returns the resulting bool state. – Francis Cugler Jul 26 '15 at 15:40
  • It's pretty much unreadable. All good coding standards will object to deeply nested conditional operators. It certainly need not be stored on the stack. It may be stored in a register. Do you not understand that local variables can reside entirely in registers? And, for correctness, the operator's name is the conditional operator. – David Heffernan Jul 26 '15 at 18:54
  • Yes I understand, and that is a matter of semantics, some call it the ternary operator and some call it the conditional operator, both are valid. – Francis Cugler Jul 26 '15 at 19:44
  • @FrancisCugler Although I applaud the effort, I agree with David on this one. The slight optimization you _may_ receive from writing the code this way is greatly offset by its lack of maintainability. Yes, your solution is semantically correct and well formatted on the page regarding white space, so that its meaning should be clear to the reader. However, should the need arise, the fact that it's a single statement excludes the possibility of adding more statements in between conditionals without rewriting this as an if else if block. – LowTechGeek Jul 26 '15 at 22:07
  • @LowTechGeek I am not trying to say that this is the best possible answer, but is another alternative solution to accomplish the same task. Personally I do use the ternary statements, but I do not normally nest them this deep. I may nest 2-3 at most. It all depends on the situation and the need for it. A good time to use a ternary or conditional operator is as a parameter to a function call when that parameter is unknown and has only 2 possible choices depending on some other calculation. It does how ever solve the issue of not having more then 3 nested statements and a single return value. – Francis Cugler Jul 27 '15 at 16:40
1
if (lhs.a != rhs.a) retval = lhs.a < rhs.a; goto end;
if (lhs.b != rhs.b) retval = lhs.b < rhs.b; goto end;
if (lhs.c != rhs.c) retval = lhs.c < rhs.c; goto end;
if (lhs.d != rhs.d) retval = lhs.d < rhs.d; goto end;
if (lhs.e != rhs.e) retval = lhs.e < rhs.e; goto end;
retval = lhs.f < rhs.f
end:
return retval;
phuclv
  • 37,963
  • 15
  • 156
  • 475