For-if antipattern

Je lisais sur ce billet de blog sur l'anti-modèle for-if, et je ne suis pas sûr de comprendre pourquoi c'est un anti-modèle.

foreach (string filename in Directory.GetFiles("."))
{
    if (filename.Equals("desktop.ini", StringComparison.OrdinalIgnoreCase))
    {
        return new StreamReader(filename);
    }
}

Question 1:

Est-ce à cause de l' return new StreamReader(filename);intérieur du for loop? ou le fait que vous n'avez pas besoin d'une forboucle dans ce cas?

Comme l'a souligné l'auteur du blog, la version la moins folle de ceci est:

if (File.Exists("desktop.ini"))
{
    return new StreamReader("desktop.ini");
} 

les deux souffrent d'une condition de concurrence car si le fichier est supprimé avant la création du StreamReader, vous obtiendrez un File­Not­Found­Exception.

Question 2:

Pour corriger le deuxième exemple, le réécrivez-vous sans l'instruction if, et entourez-le StreamReaderà la place d'un bloc try-catch, et s'il le lance, File­Not­Found­Exceptionvous le gérez dans le catchbloc en conséquence?

Ceci est un contre-modèle car il prend la forme de:

loop over a set of values
   if current value meets a condition
       do something with value
   end
end

et peut être remplacé par

do something with value

Un exemple classique de ceci est un code comme:

for (var i=0; i < 5; i++)
{
    switch (i)
        case 1:
            doSomethingWith(1);
            break;
        case 2:
            doSomethingWith(2);
            break;
        case 3:
            doSomethingWith(4);
            break;
        case 4:
            doSomethingWith(4);
            break;
    }
}

Lorsque ce qui suit fonctionne très bien:

doSomethingWith(1);
doSomethingWith(2);
doSomethingWith(3);
doSomethingWith(4);

Si vous vous retrouvez à exécuter une boucle et un ifou switch, alors arrêtez-vous et pensez à ce que vous faites. Êtes-vous en train de trop compliquer les choses et la boucle et le test peuvent-ils être remplacés simplement par une simple ligne "faites-le"? Parfois, cependant, vous constaterez que vous devez effectuer cette boucle (plusieurs éléments peuvent correspondre à la seule condition, par exemple), auquel cas le modèle est correct.

C'est pourquoi c'est un anti-pattern: il prend le pattern "loop and test" et en abuse.

Concernant votre deuxième question: oui. Un modèle «essayer» est plus robuste qu'un modèle «tester puis faire» dans toutes les situations où votre code n'est pas le seul thread sur l'ensemble de l'appareil qui peut changer l'état de l'élément en cours de test.

Le problème avec ce code:

if (File.Exists("desktop.ini"))
{
    return new StreamReader("desktop.ini");
}

est que dans l'intervalle entre le File.Existset la StreamReadertentative d'ouverture de ce fichier, un autre thread ou processus pourrait supprimer le fichier. Vous obtiendrez donc une exception. Par conséquent, cette exception doit être protégée contre quelque chose comme:

try
{
    return new StreamReader("desktop.ini");
}
catch (File­Not­Found­Exception)
{
    return null; // or whatever
}

@Flater soulève un bon point? Est-ce lui-même un contre-modèle? Utilisons-nous des exceptions comme flux de contrôle?

Si le code lit quelque chose comme:

try
{
    if (!File.Exists("desktop.ini")
    {
        throw new IniFileMissingException();
        return new StreamReader("desktop.ini");
    }
}
catch (IniFileMissingException)
{
    return null;
}

alors j'utiliserais en effet des exceptions comme un goto glorifié et ce serait en effet un anti-modèle. Mais dans ce cas, nous travaillons simplement sur le comportement indésirable de la création d'un nouveau flux, donc ce n'est pas un exemple de cet anti-modèle. Mais c'est un exemple de contourner cet anti-modèle.

Bien sûr, ce que nous voulons vraiment, c'est une façon plus élégante de créer le flux. Quelque chose comme:

return TryCreateStream("desktop.ini", out var stream) ? stream : null;

Et je recommanderais d'envelopper ce try catchcode dans une méthode utilitaire comme celle-ci si vous vous retrouvez à utiliser beaucoup ce code.

Question 1: (est-ce pour une boucle un contre-modèle)

Oui, car vous n'avez pas besoin de faire votre propre recherche pour les éléments stockés dans un sous-système interrogeable.

Dans l'abstrait, le système de fichiers, comme une base de données, est capable de répondre aux requêtes. Lorsque nous interagissons avec un sous-système interrogeable, nous avons un choix fondamental quant à savoir si nous voulons qu'un tel sous-système nous énumère son contenu afin que nous effectuions la correspondance en dehors du sous-système, ou pour utiliser les capacités de requête native du sous-système.

Imaginons un instant que vous recherchez un enregistrement dans une base de données au lieu d'un fichier dans un répertoire d'un système de fichiers. Préférez-vous voir

SELECT * FROM SomeTable;

puis en boucle (par exemple en C #) sur le curseur renvoyé à la recherche d'ID = 100, ou en laissant le sous-système interrogeable faire ce qu'il peut pour trouver exactement ce que vous recherchez à la place?

SELECT * FROM SomeTable WHERE ID = 100;

Je pense que la plupart d'entre nous choisiraient à juste titre de laisser le sous-système effectuer la requête exacte qui l'intéresse. L'alternative implique potentiellement de nombreux allers-retours avec le sous-système, des tests d'égalité inefficaces et la renonciation à l'utilisation d'index ou d'autres accélérateurs de recherche, que nous fournissent les bases de données et les systèmes de fichiers.

Question 2: Pour corriger le deuxième exemple, voulez-vous le réécrire sans l'instruction if, et entourer à la place le StreamReader avec un bloc try-catch, et s'il lève une FileNotFoundException, vous le gérez en conséquence dans le bloc catch?

Oui, car c'est ainsi que fonctionne cette API particulière - ce n'est pas vraiment notre choix car il s'agit d'une fonction de bibliothèque. La vérification if, avant l'appel, n'offre aucune valeur supplémentaire: nous devons quand même utiliser le try / catch, car (1) d'autres erreurs au-delà de FileNotFound peuvent se produire, et (2) la condition de concurrence critique.

Question 1:

Est-ce à cause du retour de nouveau StreamReader (nom de fichier); à l'intérieur de la boucle for? ou le fait que vous n'avez pas besoin d'une boucle for dans ce cas?

Le streamreader n'a rien à voir avec cela. L'anti-modèle émerge en raison d'un conflit d'intention clair entre le foreachet le if:

Quel est le but du foreach?

Je suppose que votre réponse sera quelque chose comme: "Je veux exécuter à plusieurs reprises un morceau de code particulier"

Combien de fichiers prévoyez-vous de traiter?

Étant donné que vous ne pouvez avoir qu'un seul nom de fichier spécifique (y compris l'extension) dans un dossier particulier, cela prouve que votre code est destiné à trouver un fichier applicable.

Cela est également confirmé par le fait que vous renvoyez une valeur immédiatement. Vous ne vous souciez pas réellement d'un deuxième match, même s'il devait exister.

Il y a des situations où ce n'est pas un anti-modèle.

• Si vous regardez également dans les sous-répertoires ( Directory.GetFiles(".", SearchOption.AllDirectories)), il est possible de trouver plus d'un fichier avec le même nom de fichier (y compris l'extension)
• Si vous recherchez des correspondances de nom de fichier partielles (par exemple, chaque fichier dont le nom commence par "Test_", ou chaque "*.zip"fichier.

Notez que ces deux cas nécessitent que vous traitez réellement plusieurs correspondances et que vous ne renvoyiez donc pas de valeur immédiatement.

Question 2:

Pour corriger le deuxième exemple, voulez-vous le réécrire sans l'instruction if, et à la place entourer le StreamReader d'un bloc try-catch, et s'il lève une FileNotFoundException, vous le gérez en conséquence dans le bloc catch?

Les exceptions coûtent cher. Ils ne doivent jamais être utilisés à la place d'une logique de flux appropriée. Les exceptions sont, comme leur nom l'indique, des circonstances exceptionnelles .

Pour cette raison, vous ne devez pas supprimer le fichier if.

Selon cette réponse sur SoftwareEngineering.SE :

Généralement, l'utilisation d'exceptions pour le flux de contrôle est un anti-modèle, avec des toux exceptionnelles spécifiques à la situation et à la langue.

En résumé, pourquoi, généralement, c'est un anti-modèle:

• Les exceptions sont, par essence, des instructions GOTO sophistiquées
• La programmation avec des exceptions conduit donc à un code plus difficile à lire et à comprendre
• La plupart des langues ont des structures de contrôle existantes conçues pour résoudre vos problèmes sans utiliser d'exceptions
• Les arguments en faveur de l'efficacité ont tendance à être sans objet pour les compilateurs modernes, qui ont tendance à optimiser en supposant que les exceptions ne sont pas utilisées pour le flux de contrôle.

Lisez la discussion sur le wiki de Ward pour des informations beaucoup plus approfondies.

Que vous ayez besoin d'envelopper dans un essai / capture, cela dépend fortement de votre situation:

• Quelle est la probabilité que vous rencontriez une condition de concurrence?
• Êtes-vous réellement capable de gérer cette situation, ou voulez-vous que ce problème se propage à l'utilisateur parce que vous ne savez pas comment le gérer.

Il ne s'agit jamais de "toujours l'utiliser". Pour prouver mon point:

Des études distinctes ont prouvé que vous êtes moins susceptible de vous blesser lorsque vous portez un casque de sécurité, des lunettes de sécurité et des gilets pare-balles.

Alors pourquoi ne portons-nous pas tous cet équipement de sécurité en tout temps?

La réponse est simple car il y a des inconvénients à les porter:

• Ça coûte de l'argent
• Cela rend votre mouvement plus encombrant
• Il peut être assez chaud de le porter.

Nous arrivons maintenant quelque part: il y a des avantages et des inconvénients . En d'autres termes, il est logique de porter cet équipement dans les cas où les avantages l' emportent sur les inconvénients.

• Les travailleurs de la construction sont beaucoup plus susceptibles de subir une blessure au cours de leur travail. Ils bénéficient d'un casque de sécurité.
• Les employés de bureau, en revanche, ont beaucoup moins de chances de se blesser. Les casques de sécurité n'en valent pas la peine.
• Un membre de l'équipe SWAT est beaucoup plus susceptible de se faire tirer dessus, qu'un employé de bureau.

Devriez-vous terminer l'appel dans un essai / capture? Cela dépend beaucoup de la question de savoir si les avantages de le faire l'emportent sur le coût de sa mise en œuvre.

Notez que d'autres peuvent faire valoir qu'il ne faut que quelques touches pour l'envelopper, donc cela devrait évidemment être fait. Mais ce n'est pas tout l'argument:

• Vous devez décider quoi faire une fois que vous avez saisi l'exception.
• S'il y a beaucoup d'appels différents à différents fichiers partout dans la base de code, décider d'en encapsuler un dans un try / catch signifie généralement que vous devez encapsuler tous ces cas. Cela peut avoir un effet dramatique sur la quantité d'efforts nécessaires pour le mettre en œuvre.
• Il est parfaitement possible que vous souhaitiez intentionnellement ne pas gérer d'exception.
  • Notez que votre application devra gérer l'exception à un moment donné, mais ce n'est pas nécessairement immédiatement après le déclenchement de l'exception.

Donc, c'est à toi de choisir. Y a-t-il un avantage à le faire? Pensez-vous que cela améliore l'application, plus que le coût des efforts de mise en œuvre?

Mise à jour - D'un commentaire que j'ai écrit à l'autre réponse, car je pense que c'est une considération pertinente pour vous aussi:

Cela dépend beaucoup du contexte environnant.

• Si l'ouverture du streamreader est précédée de if(!File.Exists) File.Create(), alors l'absence du fichier lors de l'ouverture du streamreader est en effet exceptionnelle .
• Si le nom de fichier a été sélectionné dans une liste de fichiers existants, son absence soudaine est à nouveau exceptionnelle .
• Si vous travaillez avec une chaîne que vous n'avez pas encore testée par rapport au répertoire; alors l'absence du dossier est une issue parfaitement logique, et donc pas exceptionnelle .