Est-ce qu'une branche Si ne fait rien d'une odeur de code ou d'une bonne pratique?

Question

J'ai répondu à des discussions ici (ou au moins commented) avec des réponses contenant du code comme celui-ci, mais je me demande s'il est bon ou mauvais d'écrire une série de branches avec une (ou plusieurs) des branches rien en eux, généralement pour éliminer la vérification de null dans chaque branche.

Un exemple (code C#):

if (str == null) { /* Do nothing */ } 
else if (str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str.Length > 1) 
{ 
    // ... 
} 

au lieu de:

if (str != null && str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str != null && str.Length > 1) 
{ 
    // ... 
} 

Et, bien sûr, cela est juste un exemple, comme je l'ai tendance à les utiliser avec un complexe plus grand et plus Des classes. Et dans la plupart de ces cas, une valeur null indiquerait de ne rien faire.

Pour moi, cela réduit la complication de mon code et a du sens quand je le vois. Alors, est-ce une bonne ou une mauvaise forme (une odeur de code, même)?

Answer 1

il est en effet bon d'éviter ce qui suit, car il inutilement revérifie l'une des conditions (le fait que le compilateur d'optimiser cette distance est à côté du point - il potentiellement marques plus de travail pour les gens qui essaient de lire votre code):

if (str != null && str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str != null && str.Length > 1) 
{ 
    // ... 
} 

Mais il est plutôt bizarre de faire aussi ce que vous suggérez, ci-dessous:

if (str == null) { /* Do nothing */ } 
else if (str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str.Length > 1) 
{ 
    // ... 
} 

Je dis cela est bizarre parce qu'il obscurcit votre intention et défie les attentes du lecteur.Si vous vérifiez une condition, les gens s'attendent à ce que vous fassiez quelque chose si elle est satisfaite - mais vous ne l'êtes pas. Ceci est parce que votre intention est de ne pas réellement processus la condition nulle, mais plutôt pour éviter un pointeur NULL lorsque vous vérifiez les deux conditions que vous êtes réellement intéressé. En effet, plutôt que d'avoir deux états conceptuels handle, avec une clause de santé mentale (entrée non nulle), il lit à la place comme vous avez trois états conceptuels à gérer. Le fait que, computationnellement, vous pourriez dire qu'il y a trois tels états est à côté du point - c'est moins clair.

L'approche habituelle de cas dans ce genre de situation est comme Oren A suggéré - vérifier l'hypothèse nulle, puis vérifiez les autres conditions à l'intérieur du bloc de résultat:

if (str != null) 
{ 
if (str == "SomeSpecialValue") 
{ 
    // ... 
} 
else if (str.Length > 1) 
{ 
    // ... 
} 
} 

Ceci est un peu plus d'une question style de lisibilité-amélioration, par opposition à un problème d'odeur de code.

EDIT: Cependant, si vous êtes prêt à la condition de ne rien faire, je ne très semblable à celle que vous avez inclus un commentaire « ne rien faire ». Sinon, les gens pourraient penser que vous avez simplement oublié de compléter le code.

Answer 2

Je préfère le faire comme this-

if (str != null) 
{ 
if (str == "[NULL]") 
{ 
    // ... 
} 
else if (str.Length > 1) 
{ 
    // ... 
} 
} 

Je pense que vous pouvez toujours « reformule » un if avec un corps vide en elle est la négation avec un corps, et qu'il semble mieux et est plus logique.

Answer 3

Votre premier exemple est parfaitement lisible pour moi - ne sent pas du tout.

Answer 4

Dans ce cas particulier, je vais revenir au début et il rend le code plus facile à lire

if (string.IsNullOrEmpty(str)) { return; } 

Je tiens à mettre une déclaration de retour explicite.

Answer 5

Tout dépend du contexte. Si mettre une instruction if vide rend le code plus lisible, alors allez-y.

Answer 6

Il est lisible, que ce soit bon ou mauvais dépend de ce que vous essayez d'atteindre - généralement embêté "va-pour-toujours" de type if instructions sont mauvaises. Ne pas oublier les méthodes de chaînes statiques cuites dans le cadre: string.IsNullOrEmpty() et string.IsNullOrWhiteSpace().

Votre ligne if (str == null) { /* Do nothing */ } est inhabituelle, mais elle a un point positif: elle fait savoir aux autres développeurs que vous ne faites délibérément rien dans ce cas, avec votre longue structure if/else si vos intentions pourraient devenir floues si vous changé à

if (str != null) 
{ 
    /* carry on with the rest of the tests */ 
} 

Answer 7

Normalement je mettre un return ou quelque chose comme ça dans la première if:

void Foo() 
{ 
    if (str == null) { return; } 
    if (str == "SomeSpecialValue") 
    { 
     // ... 
    } 
    else if (str.Length > 1) 
    { 
     // ... 
    } 
} 

Si vous ne pouvez pas faire cela, parce que la fonction fait quelque chose d'autre après la if/else, je dirais Il est temps de refactoriser, et diviser la partie if/else dans une fonction distincte, à partir de laquelle vous pouvez revenir tôt.

Answer 8

Oui C'est une odeur de code.

Une indication est que vous avez pensé à poser cette question.

Une autre indication est que le code semble incomplet - comme si quelque chose devait y appartenir. Il peut être lisible, mais il se sent off. Lors de la lecture de ce code, un étranger doit s'arrêter pendant une seconde et utiliser son intelligence pour déterminer si le code est valide/complet/correct/comme prévu/adjectif.

user359996 mis le doigt sur la tête:

Je dis cela est bizarre parce qu'il obscurcit votre intention et défie les attentes du lecteur.