38

I came across this code written by someone else. Is this usage of the conditional operator recommended or commonly used? I feel it is less maintainable - or is it just me? Is there any alternate way of writing this?

  exp_rsp_status =  req.security_violation ? (dis_prot_viol_rsp && is_mstr) ? 
                    uvc_pkg::MRSP_OKAY : uvc_pkg::MRSP_PROTVIOL : req.slv_req.size()  ?
                    ((is_mst_abort_rsp && dis_mst_abort_rsp) ||
                    ((req.slv_req[0].get_rsp_status()==uvc_pkg::MRSP_PROTVIOL) && dis_prot_viol_rsp) ||
                    (is_mst_abort_rsp && req.is_pci_config_req() && dis_pcicfg_mst_abort_rsp)) ?
                    uvc_pkg::MRSP_OKAY : req.slv_req[0].get_rsp_status() : uvc_pkg::MRSP_OKAY;
Daniel Fischer
  • 181,706
  • 17
  • 308
  • 431
Jean
  • 21,665
  • 24
  • 69
  • 119
  • Is `exp_rsp_status` default constructable and assignable? – NathanOliver Oct 20 '15 at 14:40
  • Related: http://stackoverflow.com/questions/18237432/how-to-rewrite-complicated-lines-of-c-code-nested-ternary-operator or http://stackoverflow.com/questions/19020092/multiple-ternary-operator-in-c – Fabio says Reinstate Monica Oct 20 '15 at 15:03
  • Your question focuses on the ternary operator when the real problem here is that it's simply way too complex, ternary or not. – Loren Pechtel Oct 21 '15 at 03:10
  • 5
    I removed the [bad-code] tag here because it's a 'meta tag' - a tag that describes what the question *is*, rather than what it's about. You might consider asking [on meta](http://meta.stackoverflow.com) if you'd like to discuss it. Thanks! – Undo Dec 10 '15 at 20:36
  • 1
    Continually rolling back edits you disagree with is not constructive. If you want to discuss wether or not an edit should stay or go you can do so on [Meta], rather than engage in a tug of war. – Martijn Pieters Dec 10 '15 at 22:32

6 Answers6

64

That's just horrible code.

  • It's badly formatted. I don't see the hierarchy of the expression.
  • Even if it had good formatting, the expression would be way too complex to quickly parse with the human eye.
  • The intention is unclear. What's the purpose of those conditions?

So what can you do?

  • Use conditional statements (if).
  • Extract the sub-expressions, and store them in variables. Check this nice example from the refactoring catalog.
  • Use helper functions. If the logic is complex, use early returns. Nobody likes deep indentation.
  • Most importantly, give everything a meaningful name. The intention should be clear why something has to be calculated.

And just to be clear: There's nothing wrong with the ternary operator. If used judiously, it often produces code that's easier to digest. Avoid nesting them though. I occasionally use a second level if the code is crystal clear, and even then I use parentheses so my poor brain doesn't have to do extra cycles decyphering the operator precedence.

Care about the readers of your code.

Community
  • 1
  • 1
Karoly Horvath
  • 94,607
  • 11
  • 117
  • 176
  • Also, if use of ternary op is intended to be efficient, you could split those conditions into (named) _inline_ functions. – Little Santi Oct 20 '15 at 16:20
  • 1
    Complete aside: I can't accept that example as "nice" when it is checking for IE running on a Mac... – jpmc26 Oct 20 '15 at 22:13
  • 1
    While it's awfully hard to figure out, with proper formatting and a bunch of sub-expression extraction the ternaries are probably fine. I've written ternaries that go this deep and they're perfectly clear because everything has good names and there's no messy math in it. – Loren Pechtel Oct 21 '15 at 02:58
  • 3
    @jpmc26 why? it is absolutely valid, mac ie was supported for 18 years! – zakius Oct 21 '15 at 09:33
  • 1
    @zakius Something can be both valid and horrifying at the same time. – jpmc26 Oct 21 '15 at 18:31
29

Perhaps this is in a device driver's message loop and the original coder, possibly 10 years ago, didn't want jumps in the code. I hope he verified that his compiler didn't implement the ternary operator with jumps!

Examining the code, my first remark is that a sequence of ternary operators is -- like all code -- better readable when adequately formatted.

That said, I'm not sure that I parsed the OP's example correctly, which speaks against it. Even a traditional nested if-else construct would be hard to verify. This expression violates the fundamental programming paradigm: Divide and Conquer.

req.security_violation
?   dis_prot_viol_rsp && is_mstr
    ?   uvc_pkg::MRSP_OKAY
    :   uvc_pkg::MRSP_PROTVIOL
:   req.slv_req.size()
    ?       is_mst_abort_rsp && dis_mst_abort_rsp
        ||      req.slv_req[0].get_rsp_status()==uvc_pkg::MRSP_PROTVIOL
            &&  dis_prot_viol_rsp
        ||  is_mst_abort_rsp && req.is_pci_config_req() && dis_pcicfg_mst_abort_rsp
        ?   uvc_pkg::MRSP_OKAY
        : req.slv_req[0].get_rsp_status()
    : uvc_pkg::MRSP_OKAY;

I wanted to check how the code looks when refactored. It sure is not shorter but I like how the speaking function names make the intent clearer (of course I guessed here). This is, to some degree, pseudo code because the variable names are probably not global so that the functions would have to have parameters, making the code less clear again. But perhaps the parameter could be a single pointer to a status or request structure or such (from which values like dis_prot_viol_rsp have been extracted). Whether or not to use a ternary when combining the different conditions is up to debate. I find it often elegant.

bool ismStrProtoViol()
{
    return dis_prot_viol_rsp && is_mstr;
}

bool isIgnorableAbort()
{
    return is_mst_abort_rsp && dis_mst_abort_rsp;
}

bool isIgnorablePciAbort()
{
    return is_mst_abort_rsp && req.is_pci_config_req() && dis_pcicfg_mst_abort_rsp;
}

bool isIgnorableProtoViol()
{
    return req.slv_req[0].get_rsp_status()==uvc_pkg::MRSP_PROTVIOL &&  dis_prot_viol_rsp;
}


eStatus getRspStatus()
{
    eStatus ret;

    if( req.security_violation )
    {
        ret = ismStrProtoViol() ?  uvc_pkg::MRSP_OKAY : uvc_pkg::MRSP_PROTVIOL;
    }
    else if(  req.slv_req.size() )
    {
        ret =       isIgnorableAbort()
                ||  isIgnorableProtoViol()
                ||  isIgnorablePciAbort()
            ? uvc_pkg::MRSP_OKAY
            : req.slv_req[0].get_rsp_status();
    else
    {
        ret = uvc_pkg::MRSP_OKAY;
    }

    return ret;
}

Finally we can exploit the fact that uvc_pkg::MRSP_OKAY is kindof the default and only overwritten under certain circumstances. This eliminates a branch. Look how after some chiseling the code's reasoning is nicely visible: If it's not a security violation look closer and check the actual request status, minus empty requests and ignorable aborts.

eStatus getRspStatus()
{
    eStatus ret = uvc_pkg::MRSP_OKAY;

    if( req.security_violation )
    {
        ret = ismStrProtoViol() ? uvc_pkg::MRSP_OKAY : uvc_pkg::MRSP_PROTVIOL;
    }
    else if(        req.slv_req.size()
                &&  !isIgnorableAbort()
                &&  !isIgnorableProtoViol()
                &&  !isIgnorablePciAbort()
            )
    {
        ret = req.slv_req[0].get_rsp_status();
    }

    return ret;
}
Peter - Reinstate Monica
  • 15,048
  • 4
  • 37
  • 62
  • 3
    I'd go one step further and return early to get rid of the `ret` variable altogether... `if(req.security_violation) { return ismStrProtoViol() ? uvc_pkg::MRSP_OKAY : uvc_pkg::MRSP_PROTVIOL; } ...`. – Michael Anderson Oct 21 '15 at 01:20
  • 1
    You don't really need a bunch of functions here. Replace each of your functions with a calculation stored into a variable. – Loren Pechtel Oct 21 '15 at 03:08
  • 2
    @MichaelAnderson Yes, it's really debatable here. I usually tend to start with early returns/multiple returns but then when the code evolves there is just one more condition and suddenly you have too many to keep track of. Early returns basically initiate implicit `else` branches ("if I didn't return..."). An explicit if/else makes that more visible.-- I also actually feared being dissed for more than one return in a coding exercise ;-). – Peter - Reinstate Monica Oct 21 '15 at 08:12
  • Not 100% sure (it's a doosey), but I think your refactored code can result in calls to `req.is_pci_config_req()` that the old code wouldn't have made. Since such calls could have side effects (time or alterations to system state), I don't think we can guarantee that the newly refactored code will operate similar to the old code. Effects could range from completely benign to catastrophic. – brian_o Oct 21 '15 at 18:22
  • 1
    Glancing at version 1 of your answer, I think the functions were a better idea because they maintained the original's short-circuiting behavior. – brian_o Oct 21 '15 at 18:54
  • @brian_o Good catch. That was not intentional -- I was not aware of the semantic shift due to switching to variables until you pointed it out. – Peter - Reinstate Monica Oct 21 '15 at 19:31
  • Interesting experience. It was a refresher that changing even a few lines of working code is a **risk,** and that **reviews** (also to my attempts; thanks) are essential -- the original code would not have passed any meaningful review to begin with. – Peter - Reinstate Monica Oct 22 '15 at 11:12
13

What an ugly mess. I broke it out into if and else's just to see what it was doing. Not much more readable, but thought I'd post it anyways. Hopefully someone else has a more elegant solution for you. But to answer your question, don't use ternaries that complicated. No one wants to do what I just did to figure out what it's doing.

if ( req.security_violation )
{
    if ( dis_prot_viol_rsp && is_mstr )
    {
        exp_rsp_status = uvc_pkg::MRSP_OKAY;
    }
    else
    {
        exp_rsp_status = uvc_pkg::MRSP_PROTVIOL;
    }
}
else if ( req.slv_req.size() )
{
    if ( ( is_mst_abort_rsp && dis_mst_abort_rsp ||
         ( req.slv_req[0].get_rsp_status() == uvc_pkg::MRSP_PROTVIOL && dis_prot_viol_rsp ) ||
         ( is_mst_abort_rsp && req.is_pci_config_req() && dis_pcicfg_mst_abort_rsp ) )
    {
        exp_rsp_status = uvc_pkg::MRSP_OKAY;
    }
    else
    {
        exp_rsp_status = req.slv_req[0].get_rsp_status();
    }

}
else
{
    exp_rsp_status = uvc_pkg::MRSP_OKAY
}
DevSolar
  • 67,862
  • 21
  • 134
  • 209
Chase Henslee
  • 3,918
  • 1
  • 18
  • 21
  • 8
    I would break out all that boolean logic into separate, well named variables so that the intent is crystal clear. – TartanLlama Oct 20 '15 at 14:57
  • 2
    Please pack it back together into ternaries, it's about as readable (i.e. not at all) and you get over it quicker. :-D – DevSolar Oct 20 '15 at 14:57
  • 1
    Yeah I think the other posters here have the right idea, break it into variables for clarity. I just posted this because I wanted to see what it was doing and thought it'd be useful for others to look at haha. – Chase Henslee Oct 20 '15 at 15:00
  • Maybe delete all the unneeded parentheses in the big logical expression? You already have separation between the different clauses by linebreaks, so parentheses are not necessary. Also, I think they are unbalanced (syntax error). – anatolyg Oct 20 '15 at 15:00
  • Yeah parenthesis could go... coding standards where I work are habit forming. I have no doubt there's a syntax error. More reason to not do this kind of logic. One missed parenthesis or in the wrong area would be almost impossible to spot and would be debugging nightmare. – Chase Henslee Oct 20 '15 at 15:02
  • 1
    @anatolyg: Objection. Parantheses are never unnecessary. Either the author or the one doing the debugging later on is *bound* to waste time referencing the operator precedence table, and / or getting it wrong. ;-) – DevSolar Oct 20 '15 at 15:04
  • @DevSolar Objection ;-). Anybody who doesn't know that `&&` has higher precedence than `||` shouldn't be close to a computer with a text interface, and anybody who doesn't know that comparisons have higher precedence than both has no business writing or maintaining such code. *I* was slowed down by looking for the proper closing brackets, thinking they must be there for a reason. – Peter - Reinstate Monica Oct 21 '15 at 08:32
  • @DevSolar: While you are right about ternaries being no less readable than `if`s (if equally considerately laid out, as in Peter Schneider’s answer, where they are much more readable than “_not at all_”), the debuggers I use do not allow me to step __through__ an expression, making it easier to debug `if`s. – PJTraill Oct 21 '15 at 12:59
  • @PJTraill: I think you misunderstood my intention with that remark. *Of course* properly-layed-out `if` / `else` is better than ternaries, not only for the reason you mentioned. (Try setting a breakpoint in the "else" part of a ternary...) My comment was referring to the use of ternary not being that code snippet's *only* problem, as the spreading-out in this answer clearly showed. ;-) – DevSolar Oct 21 '15 at 13:03
  • Extracting the `MRSP_OKAY` default value into an initialisation at the start would improve it further. – ClickRick Oct 21 '15 at 13:28
9

This is terrible code.

While it is often desirable to initialize a variable with a single expression (for example, so we can make it const), this is no excuse to write code like this. You can move the complex logic into a function and call it to initialize the variable.

void
example(const int a, const int b)
{
  const auto mything = make_my_thing(a, b);
}

In C++11 and later, you can also use a lambda to initialize a variable.

void
example(const int a, const int b)
{
  const auto mything = [a, b](){
      if (a == b)
        return MyThing {"equal"};
      else if (a < b)
        return MyThing {"less"};
      else if (a > b)
        return MyThing {"greater"};
      else
        throw MyException {"How is this even possible?"};
  }();
}
5gon12eder
  • 24,280
  • 5
  • 45
  • 92
4

Others already said how awful that code excerpt is, with nice explanations. I will just provide few more reasons why that code is bad :

  1. if you consider one "if-else" to implement exactly one feature, then it is clear how complex that code is. In your case, I can not even count number of ifs.

  2. It is obvious that your code is breaking breaking the single responsibility principle, which tells :

    ...a class or module should have one, and only one, reason to change.

  3. unit testing that would be a nightmare, which is another red flag. And I bet that your colleague didn't even try to write unit tests for that piece of code.

BЈовић
  • 62,405
  • 41
  • 173
  • 273
  • __1__. Of course you can count them, there are 4 `?`s, i.e. 4 `if`s (assuming you don’t see `|| &&` as implicit `if`s). __2__. Unclear, given that your link says the principle applies to classes and we cannot see how well the given expression is separated from code addressing other concerns. __3__. _Debugging_ would, with the debuggers I know, certainly be tiresome, as they do not provide facilities to allow one to step through an expression, but (black-box) _testing_ should be independent of the code. – PJTraill Oct 21 '15 at 13:11
  • @PJTraill LOL for counting number of IFs (I consider `?:`, `if-else` and boolean operators to be branches). Regarding point 2, it says module, and if you read the book (in the link by Robert C Martin), it will become clear that it can be applied to functions as well. – BЈовић Oct 21 '15 at 13:29
  • @PJTraill Regarding the testing, I modified the answer. I ment unit testing. – BЈовић Oct 21 '15 at 13:30
0

Common or recommended? No.

I did something similar, but I had my reasons:

  1. It was an argument into a third-party C function.
  2. I was not well versed in modern C++ at the time.
  3. I commented and formatted the f*** out of it because I knew SOMEONE besides me was going to read it...or I needed to know what it was doing years later.

  4. It was DEBUG CODE that was never going into a release.

    textprintf_ex(gw->GetBackBuffer(), font, 0, 16, WHITE, -1, "BUTTON: %s",
                       //If...                        Then Display...
                  (ButtonClicked(Buttons[STOP])    ?  "STOP"
                : (ButtonClicked(Buttons[AUTO])    ?  "AUTO" 
                : (ButtonClicked(Buttons[TICK])    ?  "TICK"
                : (ButtonClicked(Buttons[BLOCK])   ?  "BLOCK"
                : (ButtonClicked(Buttons[BOAT])    ?  "BOAT"
                : (ButtonClicked(Buttons[BLINKER]) ?  "BLINKER"
                : (ButtonClicked(Buttons[GLIDER])  ?  "GLIDER"
                : (ButtonClicked(Buttons[SHIP])    ?  "SHIP"
                : (ButtonClicked(Buttons[GUN])     ?  "GUN"
                : (ButtonClicked(Buttons[PULSAR])  ?  "PULSAR"
                : (ButtonClicked(Buttons[RESET])   ?  "RESET"
                :  /*Nothing was clicked*/            "NONE"
                )))))))))))
             );

The only reason I did not use an if-else chain was it would have made the code immense and harder to follow because all I needed to do was print a word to the screen.

Casey
  • 10,297
  • 11
  • 59
  • 88