Fonctions sur une ligne appelées une seule fois

120

Considérez une fonction sans paramètre ( edit: pas nécessairement) qui exécute une seule ligne de code et qui n’est appelée qu’une fois dans le programme (bien qu’il ne soit pas impossible que cela soit nécessaire à nouveau).

Il pourrait effectuer une requête, vérifier certaines valeurs, faire quelque chose impliquant regex ... rien d'obscur ou "hacky".

La raison en serait d'éviter les évaluations difficilement lisibles:

if (getCondition()) {
    // do stuff
}

getCondition()est la fonction d'une ligne.

Ma question est simplement: est-ce une bonne pratique? Cela me semble bien mais je ne sais pas pour le long terme ...

vemv
la source
9
Sauf si votre fragment de code provient d'un langage POO avec récepteur implicite (par exemple, ceci), ce serait certainement une mauvaise pratique, car getCondition () s'appuie très probablement sur l'état global ...
Ingo
2
Il a peut-être voulu dire que getCondition () pourrait avoir des arguments?
user606723
24
@Ingo - Certaines choses ont vraiment l'état global. L'heure actuelle, les noms d'hôte, les numéros de port, etc. sont tous des "globaux" valides. L'erreur de conception est de créer un global à partir de quelque chose qui n'est pas global par nature.
James Anderson
1
Pourquoi ne vous simplement pas en ligne getCondition? Si elle est aussi petite et rarement utilisée que vous le dites, lui donner un nom ne sert à rien.
davidk01
12
davidk01: Lisibilité du code.
WJL

Réponses:

240

Cela dépend de cette ligne. Si la ligne est lisible et concise par elle-même, la fonction peut ne pas être nécessaire. Exemple simpliste:

void printNewLine() {
  System.out.println();
}

OTOH, si la fonction donne un bon nom à une ligne de code contenant par exemple une expression complexe et difficile à lire, elle est parfaitement justifiée (pour moi). Exemple élaboré (divisé en plusieurs lignes pour la lisibilité ici):

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}
Péter Török
la source
99
+1 Le mot magique ici est "Code auto-documenté".
Konamiman
8
Un bon exemple de ce que Oncle Bob appellerait "encapsulation de conditions".
Anthony Pegram le
12
@Aditya: Rien ne dit que taxPayerc'est global dans ce scénario. Peut-être que cette classe est TaxReturnet taxPayerest un attribut.
Adam Robinson
2
De plus, il vous permet de documenter la fonction de manière à ce que javadoc puisse le saisir et le rendre public.
2
@dallin, le code propre de Bob Martin montre de nombreux exemples de code de la vie réelle. Si vous avez trop de fonctions dans une seule classe, peut-être que votre classe est trop grande?
Péter Török
67

Oui, cela peut être utilisé pour satisfaire les meilleures pratiques. Par exemple, il est préférable de faire travailler une fonction clairement nommée, même si elle ne comporte qu'une ligne, plutôt que d'avoir cette ligne de code dans une fonction plus grande et d'avoir besoin d'un commentaire d'une ligne expliquant ce qu'elle fait. En outre, les lignes de code voisines doivent effectuer des tâches au même niveau d'abstraction. Un contre-exemple serait quelque chose comme

startIgnition();
petrolFlag |= 0x006A;
engageChoke();

Dans ce cas, il est définitivement préférable de déplacer la ligne médiane dans une fonction nommée de manière sensée.

Kilian Foth
la source
15
Ce 0x006A est charmant: une constante bien connue du carburant carbonisé avec les additifs a, b et c.
Coder
2
+1 Je suis tout pour le code auto-documenté :) en passant, je pense que je suis d'accord pour maintenir un niveau d'abstraction constant par blocs, mais je ne peux pas expliquer pourquoi. Souhaitez-vous développer cela?
Vemv
3
Si vous dites simplement que petrolFlag |= 0x006A;sans aucune sorte de prise de décision, il serait préférable de simplement dire petrolFlag |= A_B_C; sans fonction supplémentaire. Vraisemblablement, engageChoke()ne devrait être appelé que s'il petrolFlagrépond à un certain critère et que cela devrait clairement indiquer «J'ai besoin d'une fonction ici». Une réponse mineure, cette réponse est fondamentalement
Tim Post
9
+1 pour indiquer que le code dans une méthode doit être dans le même niveau d'abstraction. @vemv, c'est une bonne pratique car cela facilite la compréhension du code, en évitant à votre esprit de devoir sauter entre les différents niveaux d'abstraction lors de la lecture du code. Lier les commutateurs de niveau d'abstraction aux appels / retours de méthode (c'est-à-dire des "sauts" structurels) est un bon moyen de rendre le code plus fluide et plus propre.
Péter Török le
21
Non, c'est une valeur complètement inventée. Mais le fait que vous vous posiez la question ne fait que souligner le point: si le code avait été dit mixLeadedGasoline(), vous n'auriez pas à le faire!
Kilian Foth le
37

Je pense que dans de nombreux cas, une telle fonction est un bon style, mais vous pouvez envisager une variable booléenne locale comme alternative, dans les cas où vous n'avez pas besoin d'utiliser cette condition ailleurs, par exemple:

bool someConditionSatisfied = [complex expression];

Cela donnera un indice au lecteur de code et évitera l’introduction d’une nouvelle fonction.

cybevnm
la source
1
Le booléen est particulièrement bénéfique si le nom de la fonction de condition est difficile ou éventuellement trompeur (par exemple IsEngineReadyUnlessItIsOffOrBusyOrOutOfService).
dbkk
2
J'ai trouvé ce conseil comme une mauvaise idée: le bool et la condition distraient l'attention du cœur de métier de la fonction. En outre, un style privilégiant les variables locales rend le refactoring plus difficile.
Wolf
1
@Wolf OTOH, je préfère cela à un appel de fonction afin de réduire la "profondeur d'abstraction" du code. OMI, sauter dans une fonction est un changement de contexte plus important qu'un couple de lignes de code explicites et directs, en particulier s'il renvoie simplement un booléen.
Kache
@Kache Je pense que cela dépend de la nature orientée objet ou non du code. Dans le cas de la programmation orientée objet, l'utilisation de la fonction membre permet une conception beaucoup plus flexible. Cela dépend vraiment du contexte ...
Wolf
23

En plus de la réponse de Peter , si cette condition doit éventuellement être mise à jour ultérieurement, encapsulez-la de la manière que vous proposez de ne disposer que d'un seul point de montage.

Suivant l'exemple de Peter, si cela

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}

devient ceci

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isMutant() 
        && (taxPayer.getNumberOfThumbs() > 2 
        || (taxPayer.getAge() > 123 && taxPayer.getEmployer().isXMan()));
}

Vous effectuez une édition unique et elle est mise à jour universellement. Maintenabilité sage, c'est un plus.

En ce qui concerne les performances, la plupart des compilateurs optimiseurs suppriment néanmoins l’appel de fonction et insèrent le petit bloc de code en ligne. Optimiser quelque chose comme ceci peut en fait réduire la taille du bloc (en supprimant les instructions nécessaires pour l'appel de fonction, le retour, etc.). Il est donc prudent de le faire même dans des conditions qui pourraient autrement empêcher l'inlining.

Stephen
la source
2
Je suis déjà conscient des avantages de garder un point d'édition unique - c'est pourquoi la question dit "... appelé une fois". Néanmoins, il est bon de connaître ces optimisations du compilateur, j'ai toujours pensé qu'ils suivaient ce genre d'instructions à la lettre.
Vemv
Fractionner une méthode pour tester une condition simple a tendance à impliquer qu'une condition peut être changée en changeant la méthode. Une telle implication est utile quand c'est vrai, mais peut être dangereuse quand ce n'est pas le cas. Par exemple, supposons que le code doive diviser des objets en objets légers, objets verts lourds et objets lourds non verts. Les objets ont une propriété rapide "sembleGreen" qui est fiable pour les objets lourds, mais peut renvoyer des faux positifs avec des objets légers; ils ont également une fonction "measureSpectralResponse" qui est lente, mais fonctionnera de manière fiable pour tous les objets.
Supercat
Le fait qu'il seemsGreenne soit pas fiable avec des objets légers sera sans importance si le code ne se soucie jamais de savoir si les objets légers sont verts. Cependant, si la définition de "léger" change, de sorte que certains objets non verts qui retournent true seemsGreenne sont pas signalés comme "légers", une telle modification de la définition de "léger" pourrait rompre le code qui teste les objets. être vert". Dans certains cas, les tests de verdissement et de pondération dans le code peuvent rendre la relation entre les tests plus claire que s'il s'agissait de méthodes distinctes.
Supercat
5

En plus de la lisibilité (ou en complément de celle-ci), cela permet d’écrire des fonctions au niveau d’abstraction approprié.

Tylermac
la source
1
J'ai bien peur de ne pas comprendre ce que vous vouliez dire ...
vendredi
1
@vemv: Je pense qu'il entend par "niveau d'abstraction approprié" que vous ne vous retrouvez pas avec un code mélangé à différents niveaux d'abstraction (c'est-à-dire ce que Kilian Foth a déjà dit). Vous ne voulez pas que votre code traite des détails apparemment insignifiants (par exemple, la formation de tous les liens dans le carburant) lorsque le code environnant envisage une vue plus globale de la situation actuelle (par exemple, un moteur). Le premier encombre le dernier et rend votre code moins facile à lire et à maintenir car vous devrez considérer tous les niveaux d'abstraction à la fois et à tout moment.
Egon
4

Ça dépend. Parfois, il est préférable d'encapsuler une expression dans une fonction / méthode, même s'il ne s'agit que d'une ligne. Si c'est compliqué à lire ou si vous en avez besoin à plusieurs endroits, je considère cela comme une bonne pratique. À long terme, il est plus facile à maintenir, car vous avez introduit un point de changement unique et une meilleure lisibilité .

Cependant, parfois, c'est juste quelque chose dont vous n'avez pas besoin. De toute façon, quand l’expression est facile à lire et / ou apparaît juste à la même place, ne l’emballez pas.

Faucon
la source
3

Je pense que si vous n'en avez que quelques-uns, c'est correct, mais le problème se pose lorsqu'il y en a beaucoup dans votre code. Et lorsque le compilateur ou l'interpréteur s'exécute (selon la langue utilisée), cette fonction est enregistrée en mémoire. Alors disons que vous en avez 3, je ne pense pas que l'ordinateur le remarquera, mais si vous commencez à avoir une centaine de ces petites choses, le système doit alors enregistrer des fonctions en mémoire qui ne sont appelées qu'une fois et ne sont pas détruites.

WojonsTech
la source
Selon la réponse de Stephen, cela pourrait ne pas toujours arriver (bien que faire confiance aveuglément à la magie des compilateurs ne peut pas être bon de toute façon)
vendredi
1
oui, cela devrait être élucidé en fonction de nombreux faits. S'il s'agit d'une langue interpolée, le système tombera à chaque fois sur les fonctions monolignes, à moins que vous n'installiez quelque chose pour le cache qui ne résoudrait pas toujours les problèmes. Maintenant, quand il s’agit de compilateurs, le jour du faible et de la position de la planète n’est important que si le compilateur est votre ami et efface votre petit bazar ou s’il va vous sembler nécessaire. Je me souviens que si vous connaissez le nombre exact de fois où une boucle sera exécutée, il est parfois préférable de la copier et de la coller autant de fois que nécessaire.
WojonsTech le
+1 pour l'alignement des planètes :) mais ta dernière phrase me semble totalement folle, est-ce que tu le fais vraiment?
Vemv
Cela dépend vraiment de la plupart du temps non, sauf si je reçois le montant exact du paiement pour vérifier si cela accélère rapidement et d'autres choses du même genre. Mais dans les anciens compilateurs, il était préférable de le copier / coller que de le quitter pour le calculer 9/10.
WojonsTech
3

J'ai récemment effectué cette opération dans une application que je refacturais, afin d'expliciter le sens réel du code sans commentaires:

protected void PaymentButton_Click(object sender, EventArgs e)
    Func<bool> HaveError = () => lblCreditCardError.Text == string.Empty && lblDisclaimer.Text == string.Empty;

    CheckInputs();

    if(HaveError())
        return;

    ...
}
Joshperry
la source
Juste curieux, de quelle langue s'agit-il?
Vemv
5
@vemv: ça ressemble à C # pour moi.
Scott Mitchell
Je préfère également les identifiants supplémentaires aux commentaires. Mais cela diffère-t-il vraiment de l' introduction d'une variable locale pour garder le ifshort? Cette approche locale (lambda) rend la PaymentButton_Clickfonction (dans son ensemble) plus difficile à lire. Le lblCreditCardErrordans votre exemple semble être un membre, il en HaveErrorest de même un prédicat (privé) valide pour l'objet. J'aurais tendance à faire abstraction de cela, mais je ne suis pas un programmeur C #, alors je résiste.
Wolf
@ Wolf Hey mec, oui. Je l'ai écrit il y a longtemps :) Je fais les choses différemment aujourd'hui. En fait, regarder le contenu de l'étiquette pour voir s'il y a une erreur me fait grincer des dents ... Pourquoi est-ce que je ne viens pas de renvoyer un bool de CheckInputs()???
Joshperry
0

Déplacer cette ligne dans une méthode bien nommée rend votre code plus facile à lire. Beaucoup d'autres ont déjà mentionné cela ("code auto-documenté"). L’autre avantage de l’intégrer dans une méthode est qu’elle facilite le test unitaire. Une fois isolé dans sa propre méthode et testé dans l'unité, vous pouvez être sûr que si / quand un bogue est trouvé, il ne le sera pas dans cette méthode.

Andrew
la source
0

Il y a déjà beaucoup de bonnes réponses, mais il y a un cas particulier qui mérite d'être mentionné .

Si votre déclaration d' une ligne nécessite un commentaire et que vous êtes en mesure d'identifier clairement (ce qui signifie: nommez) son objectif, envisagez d'extraire une fonction tout en améliorant le commentaire dans la documentation de l'API. De cette façon, l'appel de fonction est plus facile à comprendre et à comprendre.

Il est intéressant de noter que la même chose peut être faite s’il n’ya actuellement rien à faire, mais un commentaire rappelant les extensions nécessaires (dans un avenir très proche 1) ), alors ceci:

def sophisticatedHello():
    # todo set up
    say("hello")
    # todo tear down

pourrait aussi bien changé en cette

def sophisticatedHello():
    setUp()
    say("hello")
    tearDown()

1) vous devriez être vraiment sûr de cela (voir principe de YAGNI )

Loup
la source
0

Si le langage le prend en charge, j'utilise généralement des fonctions anonymes étiquetées pour accomplir cela.

someCondition = lambda p: True if [complicated expression involving p] else False
#I explicitly write the function with a ternary to make it clear this is a a predicate
if (someCondition(p)):
    #do stuff...

IMHO c'est un bon compromis car il vous donne l'avantage de la lisibilité de ne pas avoir l'expression compliquée encombrant la ifcondition tout en évitant d'encombrer l'espace de noms global / package avec de petites étiquettes à jeter. Cela a l'avantage supplémentaire que la fonction "définition" est exacte là où elle est utilisée, ce qui facilite la modification et la lecture de la définition.

Il ne doit pas s'agir uniquement de fonctions de prédicats. J'aime aussi insérer de petites fonctions comme celle-ci dans la chaudière (cela fonctionne particulièrement bien pour la génération de listes pythoniques sans encombrer la syntaxe des crochets). Par exemple, l’exemple suivant trop simplifié lorsqu’on travaille avec PIL en python

#goal - I have a list of PIL Image objects and I want them all as grayscale (uint8) numpy arrays
im_2_arr = lambda im: array(im.convert('L')) 
arr_list = [im_2_arr(image) for image in image_list]
crasic
la source
Pourquoi "lambda p: Vrai si [expression compliquée impliquant p] else False" au lieu de "lambda p: [expression compliquée impliquant p]"? 8-)
Hans-Peter Störr le
@hstoerr est dans le commentaire juste en dessous de cette ligne. J'aime faire savoir explicitement que we someCondition est un prédicat. Bien que son strictement inutile, j'écris beaucoup de scripts scientifiques lus par des gens qui ne codent pas tant que ça, je pense personnellement son plus approprié d'avoir le laconisme supplémentaire plutôt que de mes collègues confus parce qu'ils ne savent pas que [] == Falseou d' une autre équivalence pythonique similaire qui n’est pas «toujours» intuitive. C'est essentiellement une façon de signaler que certaines conditions sont, en fait, un prédicat.
Crasic
Juste pour effacer mon erreur évidente, [] != Falsemais elle []est fausse en tant que lorsque jeté sur un
booléen
@crasic: quand je ne m'attends pas à ce que mes lecteurs sachent que [] évalue à Faux, je préfère le faire len([complicated expression producing a list]) == 0, plutôt que d'utiliser True if [blah] else Falsece qui oblige encore le lecteur à savoir que [] évalue à Faux.
Lie Ryan le