Inverser une instruction IF

Donc, je programme depuis quelques années et j'ai récemment commencé à utiliser davantage ReSharper. Une des choses que ReSharper me suggère toujours est de "inverser" si "instruction pour réduire l'imbrication".

Disons que j'ai ce code:

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
    }
}

Et ReSharper suggérera que je fasse ceci:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Il semble que ReSharper suggère TOUJOURS que j’inverse les FI, quelle que soit l’imbrication. Ce problème est que j'aime bien imbriquer dans au moins CERTAINES situations. Il me semble plus facile de lire et de comprendre ce qui se passe à certains endroits. Ce n'est pas toujours le cas, mais je me sens parfois plus à l'aise en nidification.

Ma question est la suivante: outre les préférences personnelles ou la lisibilité, existe-t-il une raison de réduire l’imbrication? Y a-t-il une différence de performance ou autre chose dont je ne suis peut-être pas au courant?

Ça dépend. Dans votre exemple, avoir l’ ifinstruction non inversée n’est pas un problème, car le corps de la ifest très court. Cependant, vous souhaitez généralement inverser les déclarations lorsque les corps se développent.

Nous ne sommes que des humains et il est difficile de garder plusieurs choses à la fois dans notre tête.

Lorsque le corps d'une instruction est volumineux, l'inversion de l'instruction réduit non seulement l'imbrication, mais indique également au développeur qu'il peut oublier tout ce qui se trouve au dessus de cette instruction et se concentrer uniquement sur le reste. Il est très probable que vous utilisiez les instructions inversées pour vous débarrasser dès que possible des valeurs erronées. Une fois que vous savez qu'ils ne sont plus dans le jeu, il ne reste plus que des valeurs pouvant être traitées.

Ne pas éviter les négatifs. Les humains craignent de les analyser même lorsque le processeur les aime.

Cependant, respectez les idiomes. Nous, êtres humains, sommes des créatures d'habitude, nous préférons donc les chemins bien usés pour diriger des chemins pleins de mauvaises herbes.

foo != nullest malheureusement devenu un idiome. Je remarque à peine les parties séparées plus. Je regarde ça et je me dis: "oh, il est prudent de se faufiler foomaintenant".

Si vous voulez renverser cela (oh, un jour, s'il vous plaît), vous avez besoin d'un cas vraiment solide. Vous avez besoin de quelque chose qui permette à mes yeux de s’échapper facilement et de le comprendre en un instant.

Cependant, ce que vous nous avez donné était ceci:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Ce qui n'est même pas structurellement le même!

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    if(someGlobalObject == null) continue;
    someSideEffectObject = someGlobalObject.SomeProperty;
}

Cela ne fait pas ce que mon cerveau paresseux attendait de lui. Je pensais pouvoir continuer à ajouter du code indépendant, mais je ne le peux pas. Cela me fait penser à des choses auxquelles je ne veux pas penser maintenant. Vous avez brisé mon idiome mais vous ne m'en avez pas donné un meilleur. Tout cela parce que vous vouliez me protéger du seul négatif qui a brûlé son méchant petit moi dans mon âme.

Donc, excusez-moi, ReSharper a peut-être raison de suggérer que ce soit 90% du temps, mais lors des contrôles nuls, je pense que vous avez trouvé un cas d'angle où il vaut mieux l'ignorer. Les outils ne permettent tout simplement pas de vous apprendre ce qu'est un code lisible. Demandez à un humain. Faites un examen par les pairs. Mais ne suivez pas aveuglément l'outil.

C'est le vieil argument de savoir si une pause est pire que l'imbrication.

Les gens semblent accepter un retour rapide pour les contrôles de paramètres, mais sa méthode est mal vue.

Personnellement, j'évite de continuer, de rompre, d'aller à la vidéo etc. même si cela signifie plus d'imbrication. Mais dans la plupart des cas, vous pouvez éviter les deux.

foreach (someObject in someObjectList.where(i=>i != null))
{
    someOtherObject = someObject.SomeProperty;
}

Évidemment, la nidification rend les choses difficiles à suivre lorsque vous avez plusieurs couches

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
            }
        }
    }
}

Le pire, c'est quand vous avez les deux

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
                return;
            }
            continue;
        }
        someObject.z --;
        if(myFunctionWithSideEffects(someObject) > 6) break;
    }
}

Le problème avec le continueest que si vous ajoutez plus de lignes au code, la logique se complique et risque de contenir des bogues. par exemple

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    ...  imagine that there is some code here ...

    // added later by a Junior Developer
    doSomeOtherFunction(someObject);
}

Voulez-vous appeler doSomeOtherFunction()pour tout someObject, ou seulement si non-null? Avec le code d'origine, vous avez l'option et c'est plus clair. Avec lecontinue ci peut oublier "oh, ouais, là nous avons sauté nulls" et vous n'avez même pas la possibilité de l'appeler pour tous les someObjects, à moins que vous ne mettiez cet appel au début de la méthode, ce qui pourrait ne pas être possible ou correct pour d'autres raisons.

Oui, beaucoup de nidification est laide et peut-être déroutant, mais dans ce cas, j'utiliserais le style traditionnel.

Question bonus: Pourquoi y a-t-il des valeurs nulles dans cette liste pour commencer? Il serait peut-être préférable de ne jamais ajouter de valeur nulle, auquel cas la logique de traitement est beaucoup plus simple.

L'idée est le retour rapide. Au début returnd'une boucle devient tôt continue.

Beaucoup de bonnes réponses à cette question: Dois-je revenir tôt d'une fonction ou utiliser une instruction if?

Notez que bien que la question soit clôturée en raison d'opinions, plus de 430 voix sont en faveur d'un retour rapide, environ 50 voix sont "ça dépend" et 8 sont contre. Il y a donc un consensus exceptionnellement fort (ou bien, ou je compte mal, ce qui est plausible).

Personnellement, si le corps de la boucle était sujet à continuer tôt et suffisamment longtemps pour qu’il ait une importance (3-4 lignes), j’ai tendance à extraire le corps de la boucle dans une fonction où j’utilise le retour rapide au lieu de continuer tôt.

Cette technique est utile pour certifier les conditions préalables lorsque l'essentiel de votre méthode se produit lorsque ces conditions sont remplies.

Nous intervertissons souvent ifsmon travail et employons un fantôme. Auparavant, il était très fréquent qu'en examinant un PR, je puisse expliquer comment mon collègue pouvait supprimer trois niveaux de nidification! Cela arrive moins souvent depuis que nous déployons nos ifs.

Voici un exemple jouet de quelque chose qui peut être déployé

function foobar(x, y, z) {
    if (x > 0) {
        if (z != null && isGamma(y)) {
            // ~10 lines of code
            // return something
        }
        else {
           return y
        }
    }
    else {
      return y + z
    }
}

Un code comme celui-ci, où il existe UN CAS intéressant (où les autres sont simplement des retours ou de petits blocs d'instructions) est commun. Il est trivial de réécrire ce qui précède comme

function foobar(x, y, z) {
    if (x <=0) {
        return y + z
    }
    if (z == null || !isGamma(y) {
       return y
    }
    // ~ 10 lines of code
    // return something
}

Ici, notre code significatif, les 10 lignes non incluses, n'est pas triple dans la fonction. Si vous avez le premier style, vous êtes tenté de refactoriser le long bloc en une méthode ou tolérez l'indentation. C'est là que les économies entrent en jeu. À un endroit, il s'agit d'une économie triviale, mais avec de nombreuses méthodes dans de nombreuses classes, vous évitez le besoin de générer une fonction supplémentaire pour rendre le code plus propre et vous n'avez pas autant de niveaux hideux à plusieurs niveaux. indentation .

En réponse à l'exemple de code d'OP, je conviens qu'il s'agit d'un effondrement excessif. D'autant plus que dans 1To, le style que j'utilise, l'inversion provoque une augmentation de la taille du code.

Les suggestions des revendeurs sont souvent utiles, mais doivent toujours être évaluées de manière critique. Il recherche des modèles de code prédéfinis et suggère des transformations, mais il ne peut pas prendre en compte tous les facteurs qui rendent le code plus ou moins lisible pour les humains.

Toutes choses étant égales par ailleurs, moins d'imbrication est préférable, c'est pourquoi ReSharper suggérera une transformation qui réduira l'imbrication. Mais toutes les autres choses ne sont pas égales: dans ce cas, la transformation nécessite l'introduction d'un continue, ce qui signifie que le code résultant est en réalité plus difficile à suivre.

Dans certains cas, échanger un niveau d'imbrication contre un continue pourrait être une amélioration, mais cela dépend de la sémantique du code en question, ce qui dépasse la compréhension des règles mécaniques de ReSharpers.

Dans votre cas, la suggestion de ReSharper aggraverait le code. Alors ignorez-le. Ou mieux: n'utilisez pas la transformation suggérée, mais réfléchissez un peu à la manière dont le code pourrait être amélioré. Notez que vous pouvez attribuer la même variable plusieurs fois, mais que seule la dernière affectation a un effet, car la précédente serait simplement écrasée. Cela fait ressembler à un bug. La suggestion de ReSharpers ne résout pas le bogue, mais elle rend un peu plus évident le fait qu'une logique douteuse est en cours.

Je pense que le point le plus important ici est que le but de la boucle dicte la meilleure façon d’organiser le flux au sein de la boucle. Si vous filtrez certains éléments avant de parcourir les éléments restants, il continueest logique de les supprimer tôt. Toutefois, si vous effectuez différents types de traitement dans une boucle, il peut être plus logique de le rendre ifvraiment évident, comme par exemple:

for(obj in objectList) {
    if(obj == null) continue;
    if(obj.getColour().equals(Color.BLUE)) {
        // Handle blue objects
    } else {
        // Handle non-blue objects
    }
}

Notez que dans Java Streams ou d’autres idiomes fonctionnels, vous pouvez séparer le filtrage du bouclage en utilisant:

items.stream()
    .filter(i -> (i != null))
    .filter(i -> i.getColour().equals(Color.BLUE))
    .forEach(i -> {
        // Process blue objects
    });

Incidemment, c’est l’une des choses que j’aime à propos de l’inversé if ( unless) de Perl qui s’applique à des instructions simples, ce qui permet le type de code suivant, que je trouve très facile à lire:

foreach my $item (@items) {
    next unless defined $item;
    carp "Only blue items are supported! Failed on item $item" 
       unless ($item->get_colour() eq 'blue');
    ...
}