Search code examples
c++ternary-operatorconditional-operator

Optimize ternary operator


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;

Solution

  • 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.