Optimiser l’opérateur ternaire

C’est juste du code horrible.

• C’est mal formaté. Je ne vois pas la hiérarchie de l’expression.
• Même si elle avait un bon formatage, l’expression serait trop complexe pour être analysée rapidement avec l’œil humain.
• L’intention n’est pas claire. Quel est le but de ces conditions?

Alors que peux-tu faire?

• Utilisez des instructions conditionnelles ( if ).
• Extrayez les sous-expressions et stockez-les dans des variables. Consultez ce bel exemple du catalogue de refactoring.
• Utilisez les fonctions d’assistance. Si la logique est complexe, utilisez les return anticipés s. Personne n’aime l’indentation profonde.
• Plus important encore, donnez un nom significatif à tout. L’intention doit être claire pour savoir pourquoi quelque chose doit être calculé.

Et juste pour être clair: il n’y a rien de mal avec l’opérateur ternaire. S’il est utilisé avec discernement, il produit souvent un code plus facile à digérer. Évitez de les imbriquer cependant. J’utilise occasionnellement un deuxième niveau si le code est limpide, et même alors j’utilise des parenthèses pour que mon pauvre cerveau n’ait pas à faire de cycles supplémentaires pour décrypter la priorité de l’opérateur.

Attention aux lecteurs de votre code.

Peut-être cela se trouve-t-il dans la boucle de messages d’un pilote de périphérique et le codeur d’origine, peut-être il y a 10 ans, ne voulait pas de sauts de code. J’espère qu’il a vérifié que son compilateur n’avait pas implémenté l’opérateur ternaire avec des sauts!

En examinant le code, ma première remarque est qu’une séquence d’opérateurs ternaires est, comme tout code, plus lisible si elle est correctement formatée.

Cela dit, je ne suis pas sûr d’avoir analysé correctement l’exemple de l’OP, ce qui est contre. Même une construction si nestede traditionnelle serait difficile à vérifier. Cette expression viole le paradigme fondamental de la programmation: Diviser et Conquérir.

 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; 

Je voulais vérifier l’apparence du code lors de la refactorisation. Ce n’est certainement pas plus court mais j’aime bien la façon dont les noms des fonctions parlantes rendent l’intention plus claire (bien sûr, je l’ai deviné ici). C’est, dans une certaine mesure, un pseudo-code car les noms de variables ne sont probablement pas globaux, de sorte que les fonctions doivent avoir des parameters, ce qui rend le code moins clair à nouveau. Mais peut-être que le paramètre pourrait être un simple pointeur vers un statut ou une structure de requête ou autre (à partir duquel des valeurs comme dis_prot_viol_rsp ont été extraites). Utiliser ou non un ternaire en combinant les différentes conditions est à débattre. Je trouve ça souvent élégant.

 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; } 

Enfin, nous pouvons exploiter le fait que uvc_pkg::MRSP_OKAY est une uvc_pkg::MRSP_OKAY défaut et ne peut être écrasé que dans certaines circonstances. Cela élimine une twig. Regardez comment, après quelques ciselures, le raisonnement du code est bien visible: si ce n’est pas une violation de la sécurité, regardez de plus près et vérifiez le statut de la requête, moins les demandes vides et les avortements ignorables.

 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() && !isIgnorablePorotoViol() && !isIgnorablePciAbort() ) { ret = req.slv_req[0].get_rsp_status(); } return ret; } 

Quel sale gâchis. Je me suis éclaté dans si et juste pour voir ce que c’était en train de faire. Pas beaucoup plus lisible, mais pensais que je le posterais quand même. J’espère que quelqu’un d’autre a une solution plus élégante pour vous. Mais pour répondre à votre question, n’utilisez pas des ternaires compliqués. Personne ne veut faire ce que je viens de faire pour comprendre ce qu’il fait.

 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 } 

C’est du code terrible.

Bien qu’il soit souvent souhaitable d’initialiser une variable avec une seule expression (par exemple, pour pouvoir la rendre const ), ce n’est pas une excuse pour écrire un code comme celui-ci. Vous pouvez déplacer la logique complexe dans une fonction et l’appeler pour initialiser la variable.

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

En C ++ 11 et versions ultérieures, vous pouvez également utiliser un lambda pour initialiser une 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?"}; }(); } 

D’autres ont déjà dit à quel point l’extrait de code était terrible, avec de belles explications. Je vais juste donner quelques autres raisons pour lesquelles ce code est mauvais:

1. Si vous considérez un “if-else” pour implémenter exactement une entité, il est clair que ce code est complexe. Dans votre cas, je ne peux même pas compter le nombre de si.

2. Il est évident que votre code enfreint le principe de la responsabilité unique , qui dit:

   … une classe ou un module devrait avoir une et une seule raison de changer.

3. test unitaire qui serait un cauchemar, qui est un autre drapeau rouge. Et je parie que votre collègue n’a même pas essayé d’écrire des tests unitaires pour ce morceau de code.

Commun ou recommandé? Non.

J’ai fait quelque chose de similaire, mais j’avais mes raisons:

1. C’était un argument dans une fonction C tierce.
2. Je ne connaissais pas bien le C ++ moderne à l’époque.
3. J’ai commenté et formaté le f *** parce que je savais que quelqu’un d’autre que moi allait le lire … ou j’avais besoin de savoir ce que ça faisait des années plus tard.

4. C’était DEBUG CODE qui n’allait jamais en sortie.

    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" ))))))))))) ); 

La seule raison pour laquelle je n’utilisais pas de chaîne if-else était que cela aurait rendu le code plus lourd et plus difficile à suivre car tout ce que j’avais à faire était d’imprimer un mot à l’écran.