Maintenabilité de la logique booléenne - L'imbrication est-elle nécessaire si des instructions sont nécessaires?

Lequel de ceux-ci est le meilleur pour la maintenabilité?

if (byteArrayVariable != null)
    if (byteArrayVariable .Length != 0)
         //Do something with byteArrayVariable 

OU

if ((byteArrayVariable != null) && (byteArrayVariable.Length != 0))
  //Do something with byteArrayVariable 

Je préfère lire et écrire le second, mais je me souviens avoir lu dans le code complet que faire des choses comme ça est mauvais pour la maintenabilité.

Cela est dû au fait que vous comptez sur la langue pour ne pas évaluer la deuxième partie du ifsi la première partie est fausse et que toutes les langues ne le font pas. (La deuxième partie lèvera une exception si elle est évaluée avec une valeur nulle byteArrayVariable.)

Je ne sais pas si c'est vraiment quelque chose à craindre ou non, et j'aimerais avoir des commentaires généraux sur la question.

Merci.

Je pense que la deuxième forme est très bien, et représente également plus clairement ce que vous essayez de faire. Vous dites que...

Je me souviens avoir lu dans le code complet que faire des choses comme ça est mauvais pour la maintenabilité. C'est parce que vous comptez sur la langue pour ne pas évaluer la deuxième partie du if si la première partie est fausse et que toutes les langues ne le font pas.

Peu importe si toutes les langues le font. Vous écrivez dans une langue particulière, donc cela n'a d'importance que si cette langue le fait. Sinon, vous dites essentiellement que vous ne devriez pas utiliser les fonctionnalités d'une langue particulière car d'autres langues pourraient ne pas prendre en charge ces fonctionnalités, et c'est tout simplement idiot.

Je suis surpris que personne ne l'ait mentionné (alors j'espère que je ne lis pas mal la question) - mais les deux sont nulles!

La meilleure alternative serait d'utiliser des sorties anticipées (en plaçant une instruction de retour au début du code si aucun autre code ne devait s'exécuter). Cela suppose que votre code est relativement bien refactorisé et que chaque méthode a un objectif concis.

À ce stade, vous feriez quelque chose comme:

if ((byteArrayVariable == null) || (byteArrayVariable.Length != 0)) {
  return; // nothing to be done, leaving
}

// Conditions met, do something

Cela pourrait ressembler beaucoup à la validation - et c'est précisément ce que c'est. Il valide que les conditions ont été remplies pour que la méthode fasse son travail - les deux options que vous avez proposées ont caché la logique réelle dans les vérifications de précondition.

Si votre code contient beaucoup de spaghettis (méthodes loooong, beaucoup d'if imbriqués avec une logique dispersée entre eux, etc.), vous devez vous concentrer sur le plus gros problème de mise en forme de votre code avant les petits problèmes stylistiques. En fonction de votre environnement et de la gravité du désordre, il existe d'excellents outils pour vous aider (par exemple, Visual Studio a une fonction de refactoring "Extract Method").

Comme Jim le mentionne, il y a encore place pour un débat sur la façon de structurer la sortie anticipée. L'alternative à ce qui précède serait:

if (byteArrayVariable == null) 
  return; // nothing to be done, leaving

if (byteArrayVariable.Length != 0)
  return;

// Conditions met, do something

Ne perdez pas de temps à essayer de piéger pour chaque non cette condition. Si vous pouvez disqualifier les données ou la condition, faites-le dès que possible.

if (byteArrayVariable == null) Exit;
if (byteArrayVariable.Length == 0) Exit;
// do something

Cela vous permet d'adapter votre code beaucoup plus facilement aux nouvelles conditions

if (byteArrayVariable == null) Exit;
if (byteArrayVariable.Length == 0) Exit;
if (NewCondition == false) Exit;
if (NewerCondition == true) Exit;
// do something

Votre exemple est l'exemple parfait de pourquoi les imbriqués ifssont une mauvaise approche pour la plupart des langues qui prennent correctement en charge la comparaison booléenne. L'imbrication ifscrée une situation où votre intention n'est pas claire du tout. Faut-il que le second ifsuive immédiatement le premier? Ou est-ce simplement parce que vous n'avez pas encore mis une autre opération là-dedans?

if ((byteArrayVariable != null) && (byteArrayVariable.Length != 0))
  //Do something with byteArrayVariable 

Dans ce cas, ceux-ci doivent à peu près être ensemble. Ils agissent comme un gardien pour vous assurer que vous n'effectuez pas une opération qui entraînerait une exception. En utilisant imbriqué ifs, vous laissez à l'interprétation de la personne qui vous suit le soin de savoir si les deux représentent une garde en deux parties ou une autre logique de branchement. En les combinant en un seul, ifje le dis. Il est beaucoup plus difficile de faufiler une opération à l'intérieur de là où elle ne devrait pas être.

Je privilégierai toujours une communication claire de mon intention plutôt que l'affirmation stupide de quelqu'un que cela "ne fonctionne pas dans toutes les langues". Devinez quoi ... throwne fonctionne pas non plus dans tous les langages, cela signifie-t-il que je ne devrais pas l'utiliser lors du codage en C # ou Java et que je devrais plutôt compter sur les valeurs de retour pour signaler quand il y a eu un problème?

Cela dit, il est beaucoup plus lisible de l'inverser et de simplement revenir de la méthode si l'une ou l'autre n'est pas satisfaite comme le dit STW dans sa réponse.

Dans ce cas, vos deux clauses sont directement liées, donc la 2e forme est préférable.

S'ils n'étaient pas liés (par exemple, une série de clauses de garde sur différentes conditions), je préférerais la première forme (ou je refaçonnerais une méthode avec un nom qui représente ce que la condition composée représente réellement).

Si vous travaillez dans Delphi, la première façon est, en général, plus sûre. (Pour l'exemple donné, il n'y a pas grand-chose à gagner à utiliser des if imbriqués.)

Delphi vous permet de spécifier si les expressions booléennes doivent évaluer ou "court-circuiter", via les commutateurs du compilateur. La méthode # 1 (if imbriqués) vous permet de vous assurer que la deuxième clause s'exécute si et seulement si (et strictement après ) la première clause est évaluée à true.

Le deuxième style peut se retourner contre vous dans VBA car si le premier test est si l'objet généré, il fera toujours le deuxième test et l'erreur. Je viens de prendre l'habitude dans VBA lorsque je traite de la vérification d'objet de toujours utiliser la première méthode.

Le deuxième formulaire est plus lisible, surtout si vous utilisez {} des blocs autour de chaque clause, car le premier formulaire entraînera des blocs {} imbriqués et plusieurs niveaux d'indentation, ce qui peut être pénible à lire (vous devez compter les espaces d'indentation pour savoir où se termine chaque bloc).

Je les trouve tous les deux lisibles et je n'accepte pas l'argument selon lequel vous devriez vous soucier de la maintenabilité du code si vous deviez changer de langue.

Dans certaines langues, la deuxième option fonctionne mieux, ce serait donc le choix le plus clair.

Je suis d'accord; Je le fais de la deuxième façon. Pour moi, c'est logiquement plus clair. Le souci que certaines langues exécutent la deuxième partie même si la première est évaluée comme fausse me semble un peu idiot, car je n'ai jamais écrit de ma vie un programme fonctionnant dans "certaines langues"; mon code semble toujours exister dans un langage spécifique, qui peut gérer cette construction ou non. (Et si je ne suis pas sûr que le langage spécifique puisse le gérer, c'est quelque chose que j'ai vraiment besoin d'apprendre, n'est-ce pas.)

Dans mon esprit, le risque avec la première façon de le faire est que cela me rend vulnérable à la mise accidentelle de code en dehors de ce ifbloc imbriqué quand je ne le voulais pas. Ce qui, certes, n'est guère une grande préoccupation, mais cela semble plus raisonnable que de s'inquiéter de savoir si mon code est indépendant du langage.

Je préfère la deuxième option, car elle est plus lisible.

Si la logique que vous implémentez est plus complexe (vérifier qu'une chaîne n'est pas nulle et non vide n'est qu'un exemple), alors vous pouvez faire un refactoring utile: extraire une fonction booléenne. Je suis avec @mipadi sur la remarque "Code complet" ("peu importe si toutes les langues le font ...") Si le lecteur de votre code n'est pas intéressé à regarder à l'intérieur de la fonction extraite, alors l'ordre dans lequel les opérandes booléens sont évalués devient une question théorique pour eux.

if ((byteArrayVariable != null)
 && (byteArrayVariable.Length != 0)) {
    //Do something with byteArrayVariable 

mais c'est essentiellement la deuxième option.