Optimiser l'opérateur ternaire

Question

Je suis tombé sur ce code écrit par quelqu'un d'autre. Cette utilisation de l'opérateur conditionnel est-elle recommandée ou couramment utilisée ? J'ai l'impression qu'il est moins facile à maintenir - ou est-ce juste moi ? Existe-t-il une autre façon d'écrire ce code ?

  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;

Answer 1

C'est juste un code horrible.

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

Alors, que pouvez-vous faire ?

• Utilisez les instructions conditionnelles ( if ).
• Extraire les sous-expressions, et les stocker dans des variables. Vérifiez este Un bel exemple du catalogue de refactoring.
• Utilisez des fonctions d'aide. Si la logique est complexe, utilisez des return s. Personne n'aime les indentations profondes.
• Et surtout, donnez à chaque chose un nom significatif. L'intention doit être claire : 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é judicieusement, il produit souvent un code plus facile à digérer. Évitez cependant de les imbriquer. J'utilise occasionnellement un deuxième niveau si le code est clair comme de l'eau de roche, et même dans ce cas, j'utilise des parenthèses pour que mon pauvre cerveau n'ait pas à faire des cycles supplémentaires pour décrypter la précédence des opérateurs.

Souciez-vous des lecteurs de votre code.

Answer 2

Peut-être se trouve-t-il dans la boucle de messages d'un pilote de périphérique et le codeur d'origine, il y a peut-être 10 ans, ne voulait pas de sauts dans le code. J'espère qu'il a vérifié que son compilateur n'implémentait pas 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 lorsqu'elle est correctement formatée.

Cela dit, je ne suis pas sûr d'avoir analysé correctement l'exemple du PO, ce qui va à son encontre. Même une construction traditionnelle if-else imbriquée 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 comment le code se présente une fois remanié. Il n'est certainement pas plus court, mais j'aime la façon dont les noms de fonctions parlantes rendent l'intention plus claire (bien sûr, j'ai deviné ici). Il s'agit, dans une certaine mesure, d'un pseudo-code, car les noms de variables ne sont probablement pas globaux, de sorte que les fonctions devraient avoir des paramètres, ce qui rend le code encore moins clair. Mais peut-être que le paramètre pourrait être un pointeur unique vers une structure d'état ou de requête ou autre (à partir de laquelle des valeurs telles que dis_prot_viol_rsp ont été extraites). L'utilisation ou non d'un ternaire pour combiner les différentes conditions est à débattre. Je trouve que c'est 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 en quelque sorte la valeur par défaut et n'est écrasée que dans certaines circonstances. Cela élimine une branche. Regardez comment, après un peu de ciselage, le raisonnement du code est joliment visible : Si ce n'est pas une violation de la sécurité, regardez de plus près et vérifiez le statut réel de la demande, sans les demandes vides et les abandons 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()
                &&  !isIgnorableProtoViol()
                &&  !isIgnorablePciAbort()
            )
    {
        ret = req.slv_req[0].get_rsp_status();
    }

    return ret;
}

Answer 3

Quel horrible gâchis. Je l'ai décomposé en "si" et "autrement" juste pour voir ce qu'il faisait. Ce n'est pas beaucoup plus lisible, mais j'ai pensé le poster quand même. J'espère que quelqu'un d'autre aura une solution plus élégante pour vous. Mais pour répondre à votre question, n'utilisez pas de ternaires aussi 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
}

Answer 4

C'est un 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 tel code. 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);
}

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

Answer 5

D'autres ont déjà dit à quel point cet extrait de code est horrible, avec de belles explications. Je vais juste fournir quelques raisons supplémentaires pour lesquelles ce code est mauvais :

1. si vous considérez qu'un "if-else" met en œuvre exactement une fonctionnalité, alors la complexité de ce code est évidente. Dans votre cas, je ne peux même pas compter le nombre de "ifs".

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

   ...une classe ou un module doit avoir une, et une seule, raison de changer.

3. Le test unitaire serait un cauchemar, ce 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.