Une grande expression booléenne est-elle plus lisible que la même expression décomposée en méthodes de prédicats? [fermé]

63

Qu'est-ce qui est plus facile à comprendre, une grosse déclaration booléenne (assez complexe) ou la même déclaration divisée en méthodes de prédicats (beaucoup de code supplémentaire à lire)?

Option 1, la grande expression booléenne:

    private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
    {

        return propVal.PropertyId == context.Definition.Id
            && !repo.ParentId.HasValue || repo.ParentId == propVal.ParentId
            && ((propVal.SecondaryFilter.HasValue && context.SecondaryFilter.HasValue && propVal.SecondaryFilter.Value == context.SecondaryFilter) || (!context.SecondaryFilter.HasValue && !propVal.SecondaryFilter.HasValue));
    }

Option 2, Les conditions décomposées en méthodes de prédicats:

    private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
    {
        return MatchesDefinitionId(context, propVal)
            && MatchesParentId(propVal)
            && (MatchedSecondaryFilter(context, propVal) || HasNoSecondaryFilter(context, propVal));
    }

    private static bool HasNoSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);
    }

    private static bool MatchedSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    }

    private bool MatchesParentId(TValToMatch propVal)
    {
        return (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    }

    private static bool MatchesDefinitionId(CurrentSearchContext context, TValToMatch propVal)
    {
        return propVal.PropertyId == context.Definition.Id;
    }

Je préfère la deuxième approche, car je vois les noms de méthodes sous forme de commentaires, mais je comprends que cela pose problème, car vous devez lire toutes les méthodes pour comprendre ce que fait le code, de sorte qu'il en résume l'intention.

willem
la source
13
L'option 2 est similaire à celle recommandée par Martin Fowler dans son livre de refactoring. De plus, les noms de méthodes servent d’intention à toutes les expressions aléatoires. Le contenu des méthodes ne sont que des détails d’implémentation qui pourraient changer avec le temps.
programmeur
2
Est-ce vraiment la même expression? "Ou" a moins de priorité que "Et", Quoi qu'il en soit, le second indique votre intention, l'autre (le premier) est technique.
thepacker
3
Ce que @thepacker dit Le fait que ce soit la première façon de faire que vous commettiez une erreur est un assez bon indice que la première façon n'est pas facilement compréhensible pour un secteur très important de votre public cible. Toi même!
Steve Jessop
3
Option 3: je n'aime ni l'un ni l'autre. La seconde est ridiculement verbeuse, la première n'est pas équivalente à la seconde. Les parenthèses aident.
David Hammen
3
Cela peut être pédant, mais vous n’avez aucune if déclaration dans aucun bloc de code. Votre question concerne les expressions booléennes .
Kyle Strand

Réponses:

88

Ce qui est plus facile à comprendre

La dernière approche. Ce n'est pas seulement plus facile à comprendre, mais il est également plus facile d'écrire, de tester, de refactoriser et d'étendre. Chaque condition requise peut être découplée et traitée en toute sécurité.

c'est problématique car il faut lire toutes les méthodes pour comprendre le code

Ce n'est pas problématique si les méthodes sont nommées correctement. En fait, il serait plus facile à comprendre car le nom de la méthode décrirait le but de la condition.
Pour un spectateur if MatchesDefinitionId()est plus explicatif queif (propVal.PropertyId == context.Definition.Id)

[Personnellement, la première approche me fait mal aux yeux.]

merveille
la source
12
Si les noms de méthodes sont bons, il est également plus facile à comprendre.
BЈовић
Et s'il vous plaît, faites-les (noms de méthodes) significatifs et courts. 20+ noms de méthode chars ont mal aux yeux. MatchesDefinitionId()est limite.
Mindwin
2
@Mindwin S'il s'agit de choisir entre garder les noms de méthode "courts" et les garder significatifs, je dis de prendre ce dernier à chaque fois. Bref c'est bien, mais pas au détriment de la lisibilité.
Ajedi32
@ Ajedi32 il n'est pas nécessaire d'écrire un essai sur ce que la méthode fait sur le nom de la méthode, ni d'avoir des noms de méthode grammaticaux. Si vous gardez les normes d’abréviation claires (dans l’ensemble du groupe de travail ou de l’organisation), le nom abrégé et la lisibilité ne poseront aucun problème.
Mindwin
Utilisez la loi de Zipf: rendez les choses plus verbeuses pour décourager leur utilisation.
hoosierEE
44

S'il s'agit du seul endroit où ces fonctions de prédicat seraient utilisées, vous pouvez également utiliser des boolvariables locales :

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    bool matchesDefinitionId = (propVal.PropertyId == context.Definition.Id);
    bool matchesParentId = (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    bool matchesSecondaryFilter = (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    bool hasNoSecondaryFilter = (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);

    return matchesDefinitionId
        && matchesParentId
        && matchesSecondaryFilter || hasNoSecondaryFilter;
}

Celles-ci pourraient également être décomposées et remises en ordre pour les rendre plus lisibles, par exemple avec

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

puis en remplaçant toutes les instances de propVal.SecondaryFilter.HasValue. Une chose qui ressort immédiatement est qu’elle hasNoSecondaryFilterutilise ET logique sur les HasValuepropriétés annulées , alors qu’elle matchesSecondaryFilterutilise un ET logique sur non-négation HasValue- ce n’est donc pas exactement le contraire.

Simon Richter
la source
3
Cette solution est très bonne et j'ai certainement écrit beaucoup de code similaire. C'est très lisible. L'inconvénient, comparé à la solution que j'ai publiée, est la rapidité. Avec cette méthode, vous effectuez une pile de tests conditionnels, quoi qu’il en soit. Dans ma solution, les opérations peuvent être considérablement réduites en fonction des valeurs traitées.
BuvinJ
5
@BuvinJ Des tests tels que ceux présentés ici devraient être relativement peu coûteux. Par conséquent, à moins que certaines des conditions ne soient coûteuses ou si le code est extrêmement sensible aux performances, j'opterais pour la version la plus lisible.
svick
1
@svick Nul doute que la plupart du temps, il est peu probable que cela engendre un problème de performances. Néanmoins, si vous pouvez réduire les opérations sans perdre en lisibilité, pourquoi ne pas le faire? Je ne suis pas convaincu que cela soit beaucoup plus lisible que ma solution. Cela donne des "noms" auto-documentés aux tests - ce qui est bien ... Je pense que cela dépend du cas d'utilisation spécifique et de la compréhensibilité des tests.
BuvinJ
Ajouter des commentaires peut aussi aider à la lisibilité ...
BuvinJ
@BuvinJ Ce que j'aime vraiment dans cette solution, c'est qu'en ignorant tout, sauf la dernière ligne, je peux rapidement comprendre ce que ça fait. Je pense que c'est plus lisible.
svick
42

En général, ce dernier est préféré.

Cela rend le site d'appel plus réutilisable. Il prend en charge DRY (ce qui signifie que vous avez moins d’endroits à modifier lorsque les critères changent et que vous pouvez le faire de manière plus fiable). Et très souvent, ces sous-critères sont des éléments qui seront réutilisés indépendamment ailleurs, ce qui vous permet de le faire.

Oh, et cela rend le test unitaire beaucoup plus facile, ce qui vous donne l'assurance que vous l'avez fait correctement.

Telastyn
la source
1
Oui, bien que votre réponse devrait également traiter de la fixation de l’utilisation de repo, qui apparaît comme un champ / propriété statique, c’est-à-dire une variable globale. Les méthodes statiques doivent être déterministes et ne pas utiliser de variables globales.
David Arno
3
@DavidArno - bien que ce ne soit pas génial, cela semble être une question fondamentale. Et sans plus de code, il est plausible qu'il existe une raison semi-valable pour que la conception fonctionne de la sorte.
Telastyn
1
Oui, Jamais De Repo. J'ai dû obscurcir un peu le code, je ne veux pas partager le code client tel quel sur les interwebs :)
willem
23

Si c'est entre ces deux choix, alors le dernier est meilleur. Ce ne sont pas les seuls choix, cependant! Pourquoi ne pas diviser la fonction unique en plusieurs ifs? Recherchez des moyens de quitter la fonction pour éviter des tests supplémentaires, en émulant approximativement un "court-circuit" dans un test de ligne unique.

Ceci est plus facile à lire (vous devrez peut-être revérifier la logique de votre exemple, mais le concept reste vrai):

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    if( propVal.PropertyId != context.Definition.Id ) return false;

    if( repo.ParentId.HasValue || repo.ParentId != propVal.ParentId ) return false;

    if( propVal.SecondaryFilter.HasValue && 
        context.SecondaryFilter.HasValue && 
        propVal.SecondaryFilter.Value == context.SecondaryFilter ) return true;

    if( !context.SecondaryFilter.HasValue && 
        !propVal.SecondaryFilter.HasValue) return true;

    return false;   
}
BuvinJ
la source
3
Pourquoi ai-je eu un vote négatif pour cela quelques secondes après l'avoir posté? S'il vous plaît ajouter un commentaire lorsque vous downvote! Cette réponse fonctionne aussi rapidement et est plus facile à lire. Donc quel est le problème?
BuvinJ
2
@BuvinJ: Absolument rien à redire. Identique au code d'origine, sauf que vous n'avez pas à vous battre avec une douzaine de parenthèses et une seule ligne qui s'étend au-delà de la fin de l'écran. Je peux lire ce code de haut en bas et le comprendre immédiatement. Compte WTF = 0.
gnasher729
1
Le renvoi à un autre endroit qu'à la fin de la fonction rend le code OMI moins lisible, pas plus lisible. Je préfère un seul point de sortie. Quelques bons arguments dans les deux sens sur ce lien. stackoverflow.com/questions/36707/…
Brad Thomas
5
@ Brad thomas Je ne peux pas accepter le point de sortie unique. Cela conduit généralement à une parenthèse imbriquée profonde. Le retour termine le chemin alors pour moi, c'est beaucoup plus facile à lire.
Borjab
1
@ BradThomas Je suis entièrement d'accord avec Borjab. C’est en fait pour cette raison que j’utilise ce style plus souvent que de casser de longues instructions conditionnelles. J'avais l'habitude de me retrouver en train d'écrire du code avec des tonnes d'imbrication. Ensuite, j'ai commencé à chercher des moyens de ne jamais faire plus d'une ou deux imbrications de profondeur, et mon code est devenu BEAUCOUP plus facile à lire et à maintenir en conséquence. Si vous pouvez trouver un moyen de quitter votre fonction, faites-le dès que possible! Si vous pouvez trouver un moyen d'éviter les imbrications profondes et les conditionnelles longues, faites-le!
BuvinJ
10

J'aime mieux l'option 2, mais je suggérerais un changement structurel. Combinez les deux vérifications de la dernière ligne du conditionnel en un seul appel.

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    return MatchesDefinitionId(context, propVal)
        && MatchesParentId(propVal)
        && MatchesSecondaryFilterIfPresent(context, propVal);
}

private static bool MatchesSecondaryFilterIfPresent(CurrentSearchContext context, 
                                                    TValToMatch propVal)
{
    return MatchedSecondaryFilter(context, propVal) 
               || HasNoSecondaryFilter(context, propVal);
}

La raison pour laquelle je suggère que c'est parce que les deux vérifications sont une seule unité fonctionnelle et que l'imbrication d'une parenthèse conditionnelle est sujette aux erreurs: à la fois du point de vue de l'écriture initiale du code et du point de vue de la personne qui le lit. C'est particulièrement le cas si les sous-éléments de l'expression ne suivent pas le même modèle.

Je ne sais pas si MatchesSecondaryFilterIfPresent()c'est le meilleur nom pour la combinaison; mais rien de mieux ne vient immédiatement à l'esprit.

Dan Neely
la source
Très bien, essayer d’expliquer ce qui se fait dans les méthodes est en fait mieux que de simplement restructurer des appels.
Klaar
2

Bien qu'en C #, le code n'est pas très orienté objet. Il utilise des méthodes statiques et ce qui ressemble à des champs statiques (par exemple repo). Il est généralement admis que la statique rend votre code difficile à refactoriser et à tester, tout en limitant la capacité de réutilisation, et, pour répondre à votre question: une utilisation statique de ce type est moins lisible et moins gérable que la construction orientée objet.

Vous devriez convertir ce code en une forme plus orientée objet. Lorsque vous le faites, vous constaterez qu’il existe des emplacements judicieux pour le code qui compare les objets, les champs, etc. demande simple de comparaison ( if ( a.compareTo (b) ) { }pouvant inclure toutes les comparaisons de champs)

C # dispose d'un riche ensemble d'interfaces et d'utilitaires système permettant d'effectuer des comparaisons sur des objets et leurs champs. Au - delà de l'évidence .Equalsméthode, pour commencer, examiner IEqualityComparer, IEquatableet les services publics comme System.Collections.Generic.EqualityComparer.Default.

Erik Eidt
la source
0

Ce dernier est définitivement préféré, j'ai vu des cas avec le premier moyen et il est presque toujours impossible à lire. J'ai commis l'erreur de le faire de la première manière et on m'a demandé de le changer pour utiliser les méthodes prédicat.

Fouiner
la source
0

Je dirais que les deux sont à peu près les mêmes, SI vous ajoutez des espaces pour la lisibilité et des commentaires pour aider le lecteur sur les parties les plus obscures.

Rappelez-vous: un bon commentaire indique au lecteur ce que vous pensiez lorsque vous avez écrit le code.

Avec des changements tels que ceux que j'ai suggérés, j'adopterais probablement l'approche précédente, car elle est moins encombrée et diffuse. Les appels de sous-programmes sont comme des notes de bas de page: ils fournissent des informations utiles mais perturbent le flux de lecture. Si les prédicats étaient plus complexes, je les répartirais en méthodes distinctes afin que les concepts qu’ils incarnent puissent être construits de manière compréhensible.

Mark Wood
la source
Mérite un +1. Bonne matière à réflexion, même si ce n'est pas l'opinion populaire basée sur les autres réponses. Merci :)
willem
1
@willem Non, il ne mérite pas +1. Deux approches ne sont pas les mêmes. Les commentaires supplémentaires sont stupides et inutiles.
Février
2
Un bon code ne dépend JAMAIS des commentaires pour être compréhensible. En fait, les commentaires sont le pire encombrement qu'un code puisse avoir. Le code devrait parler pour lui-même. En outre, les deux approches que OP veut évaluer ne peuvent jamais être "à peu près identiques", quel que soit le nombre d'espaces que l'on ajoute.
wonderbell
Il vaut mieux avoir un nom de fonction significatif que de devoir lire le commentaire. Comme indiqué dans le livre "Clean Code", un commentaire est un défaut d’expression du code de projection. Pourquoi expliquer ce que vous faites alors que la fonction aurait pu le dire beaucoup plus clairement.
Borjab
0

Eh bien, s’il ya des parties que vous voudrez peut-être réutiliser, les séparer en différentes fonctions bien nommées est évidemment la meilleure idée.
Même si vous ne les réutilisez jamais, cela vous permettra peut-être de mieux structurer vos conditions et de leur attribuer une étiquette décrivant leur signification .

Maintenant, regardons votre première option, et concédons que ni l'indentation ni les sauts de ligne n'ont été utiles, et que le conditionnel n'a pas été structuré aussi bien:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal) {
    return propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue
        || repo.ParentId == propVal.ParentId
        && propVal.SecondaryFilter.HasValue == context.SecondaryFilter.HasValue
        && (!propVal.SecondaryFilter.HasValue || propVal.SecondaryFilter.Value == context.SecondaryFilter.Value);
}
Déduplicateur
la source
0

Le premier est absolument horrible. Vous avez utilisé || pour deux choses sur la même ligne; c'est soit un bogue dans votre code, soit une intention de dissimuler votre code.

    return (   (   propVal.PropertyId == context.Definition.Id
                && !repo.ParentId.HasValue)
            || (   repo.ParentId == propVal.ParentId
                && (   (   propVal.SecondaryFilter.HasValue
                        && context.SecondaryFilter.HasValue 
                        && propVal.SecondaryFilter.Value == context.SecondaryFilter)
                    || (   !context.SecondaryFilter.HasValue
                        && !propVal.SecondaryFilter.HasValue))));

C'est au moins à peu près convenablement formaté (si le formatage est compliqué, c'est parce que la condition if est compliquée), et vous avez au moins une chance de déterminer si quelque chose est absurde. Comparé à vos déchets formatés si, tout le reste est meilleur. Mais vous semblez ne pouvoir faire que des extrêmes: soit un fouillis complet d'une déclaration if, soit quatre méthodes totalement inutiles.

Notez que (cond1 && cond2) || (! cond1 && cond3) peut être écrit comme

cond1 ? cond2 : cond3

ce qui réduirait les dégâts. J'écrirais

if (propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue) {
    return true;
} else if (repo.ParentId != propVal.ParentId) {
    return false;
} else if (propVal.SecondaryFilter.HasValue) {
    return (   context.SecondaryFilter.HasValue
            && propVal.SecondaryFilter.Value == context.SecondaryFilter); 
} else {
    return !context.SecondaryFilter.HasValue;
}
gnasher729
la source
-4

Je n'aime aucune de ces solutions, elles sont à la fois difficiles à raisonner et à lire. L'abstraction de méthodes plus petites, juste pour des méthodes plus petites, ne résout pas toujours le problème.

Idéalement, je pense que vous compareriez métaprogmatiquement les propriétés, de sorte que vous n’ayez pas à définir une nouvelle méthode ou une branche à chaque fois que vous souhaitez comparer un nouvel ensemble de propriétés.

Je ne suis pas sûr de c #, mais quelque chose comme cela en javascript serait BEAUCOUP mieux et pourrait au moins remplacer MatchesDefinitionId et MatchesParentId

function compareContextProp(obj, property, value){
  if(obj[property])
    return obj[property] == value
  return false
}
utilisateur1152226
la source
1
Cela ne devrait pas être un problème pour implémenter quelque chose comme ça en C #.
Snoop
Je ne vois pas comment une combinaison booléenne de ~ 5 appels à compareContextProp(propVal, "PropertyId", context.Definition.Id)serait plus facile à lire que la combinaison booléenne du PO de ~ 5 comparaisons du formulaire propVal.PropertyId == context.Definition.Id. Il est considérablement plus long et ajoute une couche supplémentaire sans masquer la complexité du site d’appel. (si c'est le cas, je n'ai pas
voté vers le bas