Manières élégantes de gérer si (sinon) autre

161

C'est un problème mineur, mais chaque fois que je dois coder quelque chose comme cela, la répétition me dérange, mais je ne suis pas sûr qu'aucune des solutions ne soit pire.

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}
  • Y a-t-il un nom pour ce genre de logique?
  • Suis-je un peu trop TOC?

Je suis ouvert aux suggestions de codes diaboliques, ne serait-ce que par curiosité ...

Benjol
la source
8
@Emmad Kareem: deux DefaultActionappels violent le principe DRY
Abyx
Merci pour votre réponse, mais je pense que c'est OK, sauf que vous n'utilisez pas try / catch car il peut y avoir des erreurs qui ne renvoient pas de résultats et causeraient des anomalies (selon votre langage de programmation).
NoChance
20
Je pense que le principal problème ici est que vous travaillez à des niveaux d'abstraction incohérents . Le niveau d'abstraction plus élevé est: make sure I have valid data for DoSomething(), and then DoSomething() with it. Otherwise, take DefaultAction(). Les détails indispensables de l’assurance que vous avez les données pour DoSomething () sont à un niveau d’abstraction inférieur et doivent donc occuper une fonction différente. Cette fonction aura un nom dans le niveau d'abstraction le plus élevé et sa mise en œuvre sera de bas niveau. Les bonnes réponses ci-dessous répondent à ce problème.
Gilad Naor
6
S'il vous plaît spécifier une langue. Les solutions possibles, les idiomes classiques et les normes culturelles établies de longue date diffèrent d'une langue à l'autre et donneront différentes réponses à votre question Q.
Caleb
1
Vous pouvez vous référer à ce livre "Refactoring: Amélioration de la conception du code existant". Il existe plusieurs sections sur la structure if-else, pratique réellement utile.
Vacker

Réponses:

96

Extrayez-le pour séparer la fonction (méthode) et utilisez la returndéclaration suivante:

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }
}

DefaultAction();

Ou peut-être mieux, séparez le contenu et son traitement:

contents_t get_contents(name_t file)
{
    if(!FileExists(file))
        return null;

    contents = OpenFile(file);
    if(!SomeTest(contents)) // like IsContentsValid
        return null;

    return contents;
}

...

contents = get_contents(file)
contents ? DoSomething(contents) : DefaultAction();

Upd:

Pourquoi pas les exceptions, pourquoi OpenFilene pas lancer l'exception IO:
je pense que c'est vraiment une question générique, plutôt qu'une question sur le fichier IO. Des noms tels que FileExists, OpenFilepeut être source de confusion, mais si pour les remplacer par Foo, Bar, etc., - il serait plus clair que l'on DefaultActionpeut appeler aussi souvent que DoSomething, de sorte qu'il peut être le cas non exceptionnel. Péter Török a écrit à ce sujet à la fin de sa réponse.

Pourquoi il y a un opérateur conditionnel ternaire dans la deuxième variante:
S'il y aurait une balise [C ++], j'aurais écrit une ifdéclaration avec une déclaration de contentsdans sa partie condition:

if(contents_t contents = get_contents(file))
    DoSomething(contents);
else
    DefaultAction();

Mais pour les autres langages (de type C), if(contents) ...; else ...;est exactement identique à une expression avec un opérateur conditionnel ternaire, mais plus long. Parce que la partie principale du code était une get_contentsfonction, j'ai juste utilisé la version la plus courte (et le contentstype omis ). Quoi qu'il en soit, c'est au-delà de cette question.

Abyx
la source
93
+1 pour les retours multiples - lorsque les méthodes sont rendues suffisamment petites , cette approche donne les meilleurs résultats
gnat
Pas un grand fan des retours multiples, bien que je l'utilise de temps en temps. C'est assez raisonnable sur quelque chose de simple, mais ça ne va pas bien. Notre standard est de l’éviter pour des méthodes simples mais folles car les méthodes ont tendance à grossir plus qu’elles ne rétrécissent.
Brian Knoblauch
3
Plusieurs chemins de retour peuvent avoir des conséquences négatives sur les performances des programmes C ++, mettant ainsi en échec les efforts déployés par l'optimiseur pour utiliser RVO (également NRVO, à moins que chaque chemin retourne le même objet).
Functastic
Je recommanderais d'inverser la logique sur la deuxième solution: {if (file existe) {set contents; if (sometest) {retourne le contenu; }} return null; } Cela simplifie le flux et réduit le nombre de lignes.
Wedge
1
Bonjour Abyx, j'ai remarqué que vous avez intégré certains des commentaires formulés dans les commentaires suivants: merci de l'avoir fait. J'ai nettoyé tout ce qui a été abordé dans votre réponse et d'autres réponses.
56

Si le langage de programmation que vous utilisez (0) court-circuite des comparaisons binaires (c'est-à-dire si ne pas appeler SomeTestsi FileExistsretourne faux) et (1) une affectation retourne une valeur (le résultat de OpenFileest assigné à contentspuis cette valeur est passée en argument to SomeTest), vous pouvez utiliser quelque chose comme ce qui suit, mais il vous serait quand même conseillé de commenter le code en notant que le single =est intentionnel.

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

En fonction de la complexité du if, il peut être préférable d’avoir une variable flag (qui sépare le test des conditions de réussite / échec avec le code qui traite l’erreur DefaultActiondans ce cas).

gelés
la source
Voici comment je le ferais.
Anthony
13
Assez dégoutant de mettre autant de code dans une ifdéclaration, à mon avis.
moteutsch
15
Au contraire, j'aime bien ce genre d'affirmation "si quelque chose existe et répond à cette condition". +1
Gorpik
Moi aussi! Personnellement, je n'aime pas la façon dont les gens utilisent les déclarations multiples, certaines conditions ne sont pas remplies. Pourquoi ne pas inverser ces ifs et exécuter votre code s'ils sont satisfaits?
Klaar
"Si quelque chose existe et remplit cette condition" c'est bien. "Si quelque chose existe et fait quelque chose de tangent ici et répond à cette condition", OTOH, est déroutant. En d'autres termes, je n'aime pas les effets secondaires dans une condition.
Piskvor
26

Plus sérieux que la répétition de l'appel à DefaultAction est le style lui-même car le code est écrit non orthogonal (voir cette réponse pour de bonnes raisons d'écrire orthogonalement).

Pour montrer pourquoi un code non orthogonal est incorrect, considérons l'exemple d'origine, lorsqu'une nouvelle exigence nous oblige à ne pas ouvrir le fichier s'il est stocké sur un disque réseau est introduite. Eh bien, nous pourrions simplement mettre à jour le code comme suit:

if(FileExists(file))
{
    if(! OnNetworkDisk(file))
    {
        contents = OpenFile(file); // <-- prevents inclusion in if
        if(SomeTest(contents))
        {
            DoSomething(contents);
        }
        else
        {
            DefaultAction();
        }
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Mais il faut aussi ne pas ouvrir de gros fichiers de plus de 2 Go. Eh bien, nous venons de mettre à jour à nouveau:

if(FileExists(file))
{
    if(LessThan2Gb(file))
    {
        if(! OnNetworkDisk(file))
        {
            contents = OpenFile(file); // <-- prevents inclusion in if
            if(SomeTest(contents))
            {
                DoSomething(contents);
            }
            else
            {
                DefaultAction();
            }
        }
        else
        {
            DefaultAction();
        }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Il devrait être très clair que ce style de code sera un gros problème de maintenance.

Parmi les réponses écrites correctement orthogonalement figurent le deuxième exemple d'Abyx et la réponse de Jan Hudec . Je ne vais donc pas répéter cela. Soulignons simplement que l'ajout des deux exigences dans ces réponses serait simplement

if(! LessThan2Gb(file))
    return null;

if(OnNetworkDisk(file))
    return null;

(ou goto notexists;au lieu de return null;), n'affectant aucun autre code que ces lignes ajoutées . Par exemple orthogonal.

Lors du test, la règle générale devrait être de tester les exceptions, et non le cas normal .

hlovdal
la source
8
+1 pour moi. Les retours anticipés aident à éviter le motif anti-flèche. Voir codinghorror.com/blog/2006/01/flattening-arrow-code.html et lostechies.com/chrismissal/2009/05/27/… Avant de prendre connaissance de ce modèle, je m'abonnais toujours à 1 entrée / sortie par fonction théorie en raison de ce qu'on m'a appris il y a 15 ans environ. Je pense que cela rend le code tellement plus facile à lire et plus facile à gérer.
M. Moose
3
@MrMoose: votre mention de l'anti-motif en pointe de flèche répond à la question explicite de Benjol: "Existe-t-il un nom pour ce type de logique?" Postez-le comme une réponse et vous avez mon vote.
outis
C'est une excellente réponse, merci. Et @MrMoose: "antiheadhead anti pattern" répond éventuellement à ma première puce, alors oui, postez-le. Je ne peux pas promettre que je l'accepterai, mais cela mérite des votes!
Benjol
@outis. Merci. J'ai ajouté une réponse. Le modèle anti-pointe de flèche est certainement pertinent dans le poste de hlovdal et ses clauses de garde permettent de les contourner. Je ne sais pas comment vous pourriez répondre à la deuxième puce. Je ne suis pas qualifié pour diagnostiquer ça :)
M. Moose
4
+1 pour "les exceptions de test, pas le cas normal".
Roy Tinker
25

Évidemment:

Whatever(Arguments)
{
    if(!FileExists(file))
        goto notexists;
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(!SomeTest(contents))
        goto notexists;
    DoSomething(contents);
    return;
notexists:
    DefaultAction();
}

Vous avez dit que vous êtes ouvert même aux solutions perverses, alors utiliser le mal compte, non?

En fait, selon le contexte, cette solution pourrait bien être moins diabolique que le mal agissant deux fois ou le mal extra variable. Je l'ai enveloppé dans une fonction, parce que cela ne serait certainement pas correct au milieu d'une longue fonction (notamment en raison du retour au milieu). Mais la fonction longue n'est pas OK, point final.

Lorsque vous aurez des exceptions, elles seront plus faciles à lire, en particulier si vous pouvez avoir une exception OpenFile et DoSomething simplement une exception si les conditions ne sont pas remplies. Vous n'avez donc pas besoin de vérifications explicites. En revanche, en C ++, Java et C #, une exception est une opération lente. Par conséquent, du point de vue de la performance, la lecture est toujours préférable.


Note sur le "mal": La FAQ 6.15 de C ++ définit le "mal" comme suit:

Cela signifie que telle ou telle chose est quelque chose que vous devriez éviter la plupart du temps, mais pas quelque chose que vous ne devriez pas éviter tout le temps. Par exemple, vous finirez par utiliser ces choses "mauvaises" chaque fois qu'elles sont "les moins pires des alternatives perverses".

Et cela s'applique à gotodans ce contexte. La plupart du temps, les structures de contrôle de flux structurées sont préférables, mais si vous accumulez trop de leurs propres maux, comme une affectation conditionnelle, une imbrication de plus de 3 niveaux de profondeur, une duplication de code ou de longues conditions, gotopeut tout simplement se terminer up être moins diabolique.

Jan Hudec
la source
11
Mon curseur survole le bouton d'acceptation ... juste pour contrarier tous les puristes. Oooohh la tentation: D
Benjol
2
Oui oui! C'est la manière absolument "correcte" d'écrire du code. La structure du code actuel est "Si erreur, gérer erreur. Action normale. Si erreur, gérer erreur. Action normale", ce qui est exactement comme il se doit. Tout le code "normal" est écrit avec une seule indentation de niveau alors que tout le code lié à une erreur a deux niveaux d’indentation. Ainsi, le code normal ET LE PLUS IMPORTANT prend la place visuelle la plus importante et il est possible de lire très rapidement et facilement le flux de manière descendante. Acceptez certainement cette réponse.
hlovdal
2
Et un autre aspect est que le code écrit de cette façon est orthogonal. Par exemple, les deux lignes "if (! FileExists (fichier)) \ n \ tgoto notexists;" sont maintenant UNIQUEMENT liés à la gestion de cet aspect d'erreur unique (KISS) et, plus important encore, cela n'affecte aucune des autres lignes . Cette réponse stackoverflow.com/a/3272062/23118 énumère plusieurs bonnes raisons de conserver le code orthogonal.
hlovdal
5
En parlant des solutions perverses: je peux avoir votre solution sans goto:for(;;) { if(!FileExists(file)) break; contents = OpenFile(file); if(!SomeTest(contents)) break; DoSomething(contents); return; } /* broken out */ DefaultAction();
herby
4
@herby: Votre solution est plus diabolique que vous goto, car vous abusez breakd'une manière que personne ne s'attend à ce qu'elle soit maltraitée. Par conséquent, les lecteurs du code auront plus de difficultés à voir où la rupture les mènera qu'avec goto qui le dit explicitement. De plus, vous utilisez une boucle infinie qui ne fonctionnera qu'une seule fois, ce qui sera assez déroutant. Malheureusement, ce do { ... } while(0)n’est pas tout à fait lisible non plus, parce que vous voyez que c’est juste un bloc amusant quand vous arrivez à la fin et C ne supporte pas la rupture d’autres blocs (contrairement à perl).
Jan Hudec
12
function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
herby
la source
ou allez extra male et créez une méthode FileExistsAndConditionMet (fichier) supplémentaire ...
UncleZeiv
@herby SomeTestpeut avoir la même sémantique que l'existence de fichier si SomeTestle type de fichier est vérifié, par exemple, vérifie que .gif est vraiment un fichier GIF.
Abyx
1
Oui. Dépend. @Benjol sait mieux.
Herby
3
... bien sûr je voulais dire "allez-y extra extra" ... :)
UncleZeiv
2
Cela prend des raviolis aux extrémités même si je ne vais pas (et je suis extrême dans ce domaine) ... Je pense que maintenant il est bien lisible, vu le contents && f(contents). Deux fonctions pour sauver une autre?!
Herby
12

Une possibilité:

boolean handled = false;

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        handled = true;
    }
}
if (!handled)
{
    DefaultAction();
}

Bien entendu, cela rend le code légèrement plus complexe d’une manière différente. C'est donc en grande partie une question de style.

Une approche différente consisterait à utiliser des exceptions, par exemple:

try
{
    contents = OpenFile(file); // throws IO exception if file not found
    DoSomething(contents); // calls SomeTest() and throws exception on failure
}
catch(Exception e)
{
    DefaultAction();
    // and the exception should be at least logged...
}

Cela semble plus simple, mais il n’est applicable que si

  • nous savons précisément à quel type d'exceptions s'attendre, et DefaultAction()convient à chacun
  • nous nous attendons à ce que le traitement du fichier aboutisse, et un fichier manquant ou un échec SomeTest()est clairement une condition erronée; il est donc approprié de lancer une exception.
Péter Török
la source
19
Noooo ~! Ce n'est pas une variable drapeau, c'est vraiment une mauvaise façon, car cela conduit à un code complexe, difficile à comprendre (où -il-drapeau-devient-vrai) et difficile à refactoriser.
Abyx
Pas si vous le limitez à une portée aussi locale que possible. (function () { ... })()en Javascript, { flag = false; ... }en C-like, etc.
herby
+1 pour la logique des exceptions, qui pourrait très bien être la solution la plus appropriée en fonction du scénario.
Steven Jeuris
4
+1 Cette mutuelle 'Nooooo!' est drôle. Je pense que la variable de statut et le retour rapide sont raisonnables dans certains cas. Dans les routines plus complexes, je choisirais la variable status car, plutôt que d'ajouter de la complexité, elle rend la logique explicite. Aucun problème avec ça.
grossvogel
1
C'est notre format préféré où je travaille. Les 2 principales options utilisables semblent être "retours multiples" et "variables de drapeau". Ni l'un ni l'autre ne semble avoir un quelconque avantage réel en moyenne, mais les deux conviennent mieux à certaines circonstances qu'à d'autres. Je dois aller avec votre cas typique. Juste une autre guerre religieuse "Emacs" vs "Vi". :-)
Brian Knoblauch
11

C'est à un niveau d'abstraction supérieur:

if (WeCanDoSomething(file))
{
   DoSomething(contents);
}
else
{
   DefaultAction();
} 

Et cela remplit les détails.

boolean WeCanDoSomething(file)
{
    if FileExists(file)
    {
        contents = OpenFile(file);
        return (SomeTest(contents));
    }
    else
    {
        return FALSE;
    }
}
Jeanne Pindar
la source
11

Les fonctions devraient faire une chose. Ils devraient bien le faire. Ils devraient le faire seulement.
- Robert Martin dans Clean Code

Certaines personnes trouvent cette approche un peu extrême, mais elle est également très propre. Permettez-moi d'illustrer en Python:

def processFile(self):
    if self.fileMeetsTest():
        self.doSomething()
    else:
        self.defaultAction()

def fileMeetsTest(self):
    return os.path.exists(self.path) and self.contentsTest()

def contentsTest(self):
    with open(self.path) as file:
        line = file.readline()
        return self.firstLineTest(line)

Quand il dit que les fonctions doivent faire une chose, il veut dire une chose. processFile()choisit une action en fonction du résultat d'un test, et c'est tout ce qu'il fait. fileMeetsTest()combine toutes les conditions du test, et c'est tout ce qu'il fait. contentsTest()transfère la première ligne vers firstLineTest(), et c'est tout ce que cela fait.

Cela ressemble à beaucoup de fonctions, mais il se lit pratiquement comme un anglais simplifié:

Pour traiter le fichier, vérifiez s'il répond au test. Si c'est le cas, alors faites quelque chose. Sinon, prenez l'action par défaut. Le fichier satisfait au test s'il existe et réussit le test de contenu. Pour tester le contenu, ouvrez le fichier et testez la première ligne. Le test pour la première ligne ...

Certes, c'est un peu verbeux, mais notez que si un responsable ne se soucie pas des détails, il peut arrêter de lire après seulement les 4 lignes de code processFile(), et il aura toujours une bonne connaissance de haut niveau du fonctionnement de la fonction.

Karl Bielefeldt
la source
5
+1 C'est un bon conseil, mais ce qui constitue "une chose" dépend de la couche d'abstraction actuelle. processFile () est "une chose", mais deux choses: fileMeetsTest () et doSomething () ou defaultAction (). Je crains que l'aspect "une chose" ne puisse confondre les débutants qui ne comprennent pas a priori le concept.
Caleb
1
C'est un bon objectif ... C'est tout ce que j'ai à dire à ce sujet ... ;-)
Brian Knoblauch
1
Je n'aime pas passer implicitement des arguments comme des variables d'instance comme ça. Vous êtes plein de variables d'instance "inutiles" et il y a plusieurs façons de botcher votre état et de casser les invariants.
Hugomg
@Caleb, ProcessFile () fait effectivement une chose. Comme Karl le dit dans son post, il utilise un test pour décider de l'action à entreprendre et reporte la mise en œuvre réelle des possibilités d'action à d'autres méthodes. Si vous deviez ajouter de nombreuses autres actions, le critère à objectif unique de la méthode serait toujours rempli tant qu'aucune imbrication de logique ne se produit dans la méthode immédiate.
S.Robins
6

En ce qui concerne son nom, il peut facilement devenir un modèle anti-tête de flèche à mesure que votre code prend en charge de plus en plus d'exigences (comme le montre la réponse fournie à l' adresse https://softwareengineering.stackexchange.com/a/122625/33922 ) et tombe ensuite dans le piège d'avoir d'énormes sections de codes avec des instructions conditionnelles imbriquées qui ressemblent à une flèche.

Voir les liens tels que;

http://codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://lostechies.com/chrismissal/2009/05/27/anti-patterns-and-worst-practices-the-arrowhead-anti-pattern/

Il y a beaucoup plus sur ce sujet et sur d'autres anti-modèles que l'on trouve sur Google.

Quelques bons conseils que Jeff fournit sur son blog à ce sujet sont:

1) Remplacez les conditions par des clauses de garde.

2) Décomposer des blocs conditionnels en fonctions séparées.

3) Convertir les contrôles négatifs en contrôles positifs

4) Revenez toujours de manière opportuniste dès que possible de la fonction.

Voir certains commentaires sur le blog de Jeff concernant les suggestions de Steve McConnells sur les premiers retours également;

"Utilisez un retour quand il améliore la lisibilité: dans certaines routines, une fois que vous connaissez la réponse, vous souhaitez le renvoyer immédiatement à la routine d'appel. Si la routine est définie de telle sorte qu'elle ne nécessite aucun nettoyage supplémentaire détecte une erreur, ne pas revenir immédiatement signifie que vous devez écrire plus de code. "

...

"Minimisez le nombre de retours dans chaque routine: il est plus difficile de comprendre une routine quand, en la lisant au bas, vous ignorez la possibilité qu’elle revienne quelque part. Pour cette raison, utilisez les retours judicieusement - uniquement s’ils s’améliorent. lisibilité. "

J'ai toujours souscrit à la théorie 1 entrée / sortie par fonction en raison de ce que l'on m'avait appris il y a une quinzaine d'années. Je pense que cela rend le code tellement plus facile à lire et plus facile à gérer

M. Moose
la source
6

Ceci est conforme aux règles DRY, no-goto et no-multiple-return, est évolutif et lisible à mon avis:

success = FileExists(file);
if (success)
{
    contents = OpenFile(file);
    success = SomeTest(contents);
}
if (success)
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
mouviciel
la source
1
Se conformer aux normes ne signifie pas nécessairement un bon code. Je suis actuellement indécis sur cet extrait de code.
Brian Knoblauch
cela ne fait que remplacer 2 defaultAction (); avec 2 conditions identiques si et ajoute une variable drapeau qui imo est bien pire.
Ryathal
3
L'avantage d'utiliser une construction comme celle-ci est que, à mesure que le nombre de tests augmente, le code ne commence pas à imbriquer davantage de ifs dans d'autres ifs. En outre, le code permettant de gérer la casse infructueuse ( DefaultAction()) ne se trouve qu’à un seul endroit et, à des fins de débogage, il ne saute pas autour des fonctions d’aide et l’ajout de points d’arrêt aux lignes dans lesquelles la successvariable est modifiée permet de visualiser rapidement les tests passés (au-dessus du test déclenché). point d'arrêt) et ceux qui n'ont pas été testés (ci-dessous).
frozenkoi
1
Yeeaah, je suis un peu, mais je pense que je serais goût renomme successà ok_so_far:)
Benjol
Ceci est très similaire à ce que je fais quand (1) le processus est très linéaire quand tout va bien et (2) vous auriez sinon la flèche anti-motif. Toutefois, j’essaie d’éviter d’ajouter une variable supplémentaire, ce qui est généralement facile si vous pensez en termes de conditions préalables pour l’étape suivante (ce qui est légèrement différent de demander si une étape précédente avait échoué). Si le fichier existe, ouvrez-le. Si le fichier est ouvert, lisez le contenu. Si j'ai du contenu, traitez-les, sinon faites l'action par défaut.
Adrian McCarthy
3

Je l'extrais à une méthode séparée et ensuite:

if(!FileExists(file))
{
    DefaultAction();
    return;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}

DoSomething(contents);

ce qui permet aussi

if(!FileExists(file))
{
    DefaultAction();
    return Result.FileNotFound;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return Result.TestFailed;
}

DoSomething(contents);
return Result.Success;            

alors éventuellement, vous pourriez supprimer les DefaultActionappels et laisser l'exécution DefaultActionpour l'appelant:

Result OurMethod(file)
{
    if(!FileExists(file))
    {
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            
}

void Caller()
{
    // something, something...

    var result = OurMethod(file);
    // if (result == Result.FileNotFound || result == Result.TestFailed), or just
    if (result != Result.Success)        
    {
        DefaultAction();
    }
}

J'aime aussi l'approche de Jeanne Pindar .

Konrad Morawski
la source
3

Pour ce cas particulier, la réponse est assez simple ...

Il y a une condition de concurrence entre FileExistset OpenFile: que se passe-t-il si le fichier est supprimé?

La seule façon sensée de traiter ce cas particulier est de sauter FileExists:

contents = OpenFile(file);
if (!contents) // open failed
    DefaultAction();
else (SomeTest(contents))
    DoSomething(contents);

Cela résout parfaitement ce problème et rend le code plus propre.

En général: Essayez de repenser le problème et imaginez une autre solution qui évite complètement le problème.

Martin Wickman
la source
2

Une autre possibilité, si vous n'aimez pas en voir trop, consiste à abandonner complètement l'utilisation de else et à ajouter une instruction return supplémentaire. Sinon, c'est superflu sauf si vous avez besoin d'une logique plus complexe pour déterminer s'il existe plus que deux possibilités d'action.

Ainsi, votre exemple pourrait devenir:

void DoABunchOfStuff()
{
    if(FileExists(file))
    {
        DoSomethingWithFileContent(file);
        return;
    }

    DefaultAction();
}

void DoSomethingWithFileContent(file)
{        
    var contents = GetFileContents(file)

    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }

    DefaultAction();
}

AReturnType GetFileContents(file)
{
    return OpenFile(file);
}

Personnellement, cela ne me dérange pas d'utiliser la clause else car elle énonce explicitement comment la logique est supposée fonctionner et améliore ainsi la lisibilité de votre code. Certains outils d'embellissement de code préfèrent toutefois simplifier en une seule instruction if pour décourager la logique d'imbrication.

S.Robins
la source
2

Le cas présenté dans l'exemple de code peut généralement être réduit à une seule ifinstruction. Sur de nombreux systèmes, la fonction d'ouverture de fichier renverra une valeur non valide si le fichier n'existe pas déjà. Parfois, c'est le comportement par défaut; d'autres fois, il doit être spécifié via un argument. Cela signifie que le FileExiststest peut être supprimé, ce qui peut également contribuer aux conditions de concurrence résultant de la suppression de fichier entre le test d'existence et l'ouverture du fichier.

file = OpenFile(path);
if(isValidFileHandle(file) && SomeTest(file)) {
    DoSomething(file);
} else {
    DefaultAction();
}

Cela ne résout pas directement le problème du mélange de niveaux d'abstraction, car il élimine complètement le problème des tests multiples et non chaînables, bien que le fait de supprimer le test d'existence de fichier ne soit pas incompatible avec la séparation des niveaux d'abstraction. En supposant que les descripteurs de fichier non valides équivalent à "false" et que les descripteurs de fichier soient fermés lorsqu'ils sortent de la portée:

OpenFileIfSomething(path:String) : FileHandle {
    file = OpenFile(path);
    if (file && SomeTest(file)) {
        return file;
    }
    return null;
}

...

if ((file = OpenFileIfSomething(path))) {
    DoSomething(file);
} else {
    DefaultAction();
}
réduction
la source
2

Je suis d'accord avec frozenkoi, cependant, pour C # de toute façon, j'ai pensé qu'il serait utile de suivre la syntaxe des méthodes TryParse.

if(FileExists(file) && TryOpenFile(file, out contents))
    DoSomething(contents);
else
    DefaultAction();
bool TryOpenFile(object file, out object contents)
{
    try{
        contents = OpenFile(file);
    }
    catch{
        //something bad happened, computer probably exploded
        return false;
    }
    return true;
}
Biff MaGriff
la source
1

Votre code est moche parce que vous en faites trop dans une seule fonction. Vous voulez traiter le fichier ou prendre l’action par défaut, commencez donc par dire:

if (!ProcessFile(file)) { 
  DefaultAction(); 
}

Les programmeurs Perl et Ruby écrivent processFile(file) || defaultAction()

Maintenant, écrivez ProcessFile:

if (FileExists(file)) { 
  contents = OpenFile(file);
  if (SomeTest(contents)) {
    processContents(contents);
    return true;
  }
}
return false;
Kevin Cline
la source
1

Bien sûr, vous ne pouvez pas aller aussi loin dans des scénarios comme ceux-ci, mais voici une solution:

interface File<T> {
    function isOK():Bool;
    function getData():T;
}

var appleFile:File<Apple> = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Vous voudrez peut-être des filtres supplémentaires. Alors fais ceci:

var appleFile = appleStorage.get(fileURI, isEdible);
//isEdible is of type Apple->Bool and will be used internally to answer to the isOK call
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Bien que cela puisse avoir un sens aussi bien:

function eat(apple:Apple) {
     if (isEdible(apple)) 
         digest(apple);
     else
         die();
}
var appleFile = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(appleFile.getData());
else
    cry();

Quel est le meilleur? Cela dépend du problème du monde réel auquel vous êtes confronté.
Mais ce qu'il faut retenir, c'est que vous pouvez faire beaucoup avec la composition et le polymorphisme.

back2dos
la source
1

Quel est le problème avec l'évidence

if(!FileExists(file)) {
    DefaultAction();
    return;
}
contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}        
DoSomething(contents);

Cela me semble assez standard? Pour ce genre de grosse procédure où beaucoup de petites choses doivent se passer et dont l’échec empêcherait ces dernières. Les exceptions le rendent un peu plus propre si c'est une option.

Steve Bennett
la source
0

Je réalise que c’est une vieille question, mais j’ai remarqué une tendance qui n’a pas été mentionnée; principalement, définir une variable pour déterminer plus tard la / les méthode (s) que vous souhaitez appeler (en dehors de if ... else ...).

C’est un autre angle à considérer pour rendre le code plus facile à utiliser. Cela vous permet également d’appeler une autre méthode ou de modifier la méthode appropriée à appeler dans certaines situations.

Plutôt que de devoir remplacer toutes les mentions de la méthode (et peut-être manquer certains scénarios), elles sont toutes répertoriées à la fin du bloc if ... else ... et sont plus simples à lire et à modifier. J'ai tendance à utiliser cela lorsque, par exemple, plusieurs méthodes peuvent être appelées, mais dans l'imbrication si ... sinon ... une méthode peut être appelée dans plusieurs correspondances.

Si vous définissez une variable qui définit l'état, vous pouvez avoir plusieurs options profondément imbriquées et mettre à jour l'état lorsque quelque chose doit être (ou ne pas être) exécuté.

Cela pourrait être utilisé comme dans l'exemple de la question où nous vérifions si "DoSomething" s'est produit, et si ce n'est pas le cas, exécutez l'action par défaut. Ou vous pouvez avoir l'état pour chaque méthode que vous souhaitez appeler, définir le cas échéant, puis appeler la méthode applicable en dehors de if ... else ...

À la fin des instructions if if else ... imbriquées, vous vérifiez l'état et vous agissez en conséquence. Cela signifie que vous n'avez besoin que d'une seule mention d'une méthode à la place de tous les emplacements auxquels elle devrait être appliquée.

bool ActionDone = false;

if (Method_1(object_A)) // Test 1
{
    result_A = Method_2(object_A); // Result 1

    if (Method_3(result_A)) // Test 2
    {
        Method_4(result_A); // Action 1
        ActionDone = true;
    }
}

if (!ActionDone)
{
    Method_5(); // Default Action
}
Steve Padmore
la source
0

Pour réduire les SI imbriqués:

1 / retour anticipé;

2 / expression composée (sensible au court-circuit)

Donc, votre exemple peut être refactorisé comme ceci:

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
    return;
}
DefaultAction();
DQ_vn
la source
0

J'ai vu beaucoup d'exemples avec "return" que j'utilise aussi, mais parfois, je veux éviter de créer de nouvelles fonctions et utiliser une boucle à la place:

while (1) {
    if (FileExists(file)) {
        contents = OpenFile(file);
        if (SomeTest(contents)) {
           DoSomething(contents);
           break;
        } 
    }
    DefaultAction();
    break;
}

Si vous voulez écrire moins de lignes ou si vous détestez les boucles infinies comme moi, vous pouvez changer le type de boucle en "do ... while (0)" et éviter le dernier "break".

XzKto
la source
0

Que diriez-vous de cette solution:

content = NULL; //I presume OpenFile returns a pointer 
if(FileExists(file))
    contents = OpenFile(file);
if(content != NULL && SomeTest(contents))
    DoSomething(contents);
else
    DefaultAction();

J'ai supposé qu'OpenFile renvoie un pointeur, mais cela pourrait également fonctionner avec le type de valeur return en spécifiant une valeur par défaut non retournable (codes d'erreur ou quelque chose du genre).

Bien sûr, je ne m'attends pas à une action possible via la méthode SomeTest sur le pointeur NULL (mais vous ne le savez jamais), cela pourrait donc également être vu comme une vérification supplémentaire du pointeur NULL pour l'appel SomeTest (contenu).

chedi
la source
0

Clairement, la solution la plus élégante et la plus concise consiste à utiliser une macro de préprocesseur.

#define DOUBLE_ELSE(CODE) else { CODE } } else { CODE }

Ce qui vous permet d'écrire un code magnifique comme celui-ci:

if(FileExists(file))
{
    contents = OpenFile(file);
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    DOUBLE_ELSE(DefaultAction();)

Il peut être difficile de s’appuyer sur le formatage automatique si vous utilisez souvent cette technique, et certains IDE peuvent vous crier un peu plus sur ce qu’elle suppose à tort comme étant malformé. Et comme dit le proverbe, tout est un compromis, mais je suppose que ce n'est pas un mauvais prix à payer pour éviter les méfaits d'un code répété.

Peter Olson
la source
Pour certaines personnes et dans certaines langues, les macros de préprocesseur sont un code diabolique :)
Benjol
@Benjol Vous avez dit que vous étiez ouvert aux suggestions diaboliques, non? ;)
Peter Olson
oui, absolument, c'était juste votre "éviter les maux" :)
Benjol
4
C’est tellement horrible que j’ai juste eu à l’aveuglette: D
back2dos
Shirley, tu n'es pas sérieux !!!!!!
Jim In Texas
-1

Étant donné que vous avez posé des questions par curiosité et que votre question n'est pas associée à une langue spécifique (même si vous aviez clairement à l'esprit les langues impératives), il peut être intéressant de noter que les langues prenant en charge l'évaluation paresseuse permettent une approche totalement différente. Dans ces langages, les expressions ne sont évaluées que lorsque cela est nécessaire. Vous pouvez donc définir des "variables" et les utiliser uniquement lorsque cela vous semble judicieux. Par exemple, dans un langage fictif avec des structures let/ paresseuses in, vous oubliez le contrôle de flux et écrivez:

let
  contents = ReadFile(file)
in
  if FileExists(file) && SomeTest(contents) 
    DoSomething(contents)
  else 
    DefaultAction()
tokland
la source