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.
c#
readability
willem
la source
la source
if
déclaration dans aucun bloc de code. Votre question concerne les expressions booléennes .Réponses:
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é.
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.]
la source
MatchesDefinitionId()
est limite.S'il s'agit du seul endroit où ces fonctions de prédicat seraient utilisées, vous pouvez également utiliser des
bool
variables locales :Celles-ci pourraient également être décomposées et remises en ordre pour les rendre plus lisibles, par exemple avec
puis en remplaçant toutes les instances de
propVal.SecondaryFilter.HasValue
. Une chose qui ressort immédiatement est qu’ellehasNoSecondaryFilter
utilise ET logique sur lesHasValue
propriétés annulées , alors qu’ellematchesSecondaryFilter
utilise un ET logique sur non-négationHasValue
- ce n’est donc pas exactement le contraire.la source
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.
la source
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.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):
la source
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.
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.la source
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
.Equals
méthode, pour commencer, examinerIEqualityComparer
,IEquatable
et les services publics commeSystem.Collections.Generic.EqualityComparer.Default
.la source
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.
la source
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.
la source
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:
la source
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.
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
ce qui réduirait les dégâts. J'écrirais
la source
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
la source
compareContextProp(propVal, "PropertyId", context.Definition.Id)
serait plus facile à lire que la combinaison booléenne du PO de ~ 5 comparaisons du formulairepropVal.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