Les méthodes privées avec une seule référence sont-elles mauvaises?

139

En général, j'utilise des méthodes privées pour encapsuler des fonctionnalités réutilisées à plusieurs endroits de la classe. Mais parfois, j'ai une grande méthode publique qui pourrait être divisée en étapes plus petites, chacune selon sa propre méthode privée. Cela raccourcirait la méthode publique, mais je crains que forcer quiconque lisant la méthode à utiliser différentes méthodes privées ne nuise à la lisibilité.

Y a-t-il un consensus à ce sujet? Est-il préférable d'avoir de longues méthodes publiques ou de les diviser en petites pièces même si chaque pièce n'est pas réutilisable?

Jordak
la source
7
Toutes les réponses sont basées sur l'opinion. Les auteurs originaux pensent toujours que leur code de ravioli est plus lisible. les éditeurs suivants l’appellent les raviolis.
Frank Hileman
5
Je vous suggère de lire le code propre. Il recommande ce style et a beaucoup d'exemples. Il propose également une solution au problème du "saut".
Kat
1
Je pense que cela dépend de votre équipe et des lignes de code contenues.
Espérons-
1
L'ensemble du cadre de STRUTS n'est-il pas basé sur des méthodes Getter / Setter à usage unique, qui sont toutes des méthodes à usage unique?
Zibbobz

Réponses:

203

Non, ce n'est pas un mauvais style. En fait c'est un très bon style.

Il n’est pas nécessaire que les fonctions privées existent simplement pour des raisons de réutilisation C’est certainement une bonne raison de les créer, mais il en existe une autre: la décomposition.

Considérons une fonction qui en fait trop. Il compte cent lignes et il est impossible de raisonner.

Si vous divisez cette fonction en morceaux plus petits, elle "effectue" toujours autant de travail qu'auparavant, mais en morceaux plus petits. Il appelle d'autres fonctions qui devraient avoir des noms descriptifs. La fonction principale se lit presque comme un livre: faites A, puis B, puis C, etc. Les fonctions qu’elle appelle ne peuvent être appelées qu’à un seul endroit, mais elles sont maintenant plus petites. Toute fonction particulière est nécessairement séparée des autres fonctions par un bac à sable: elles ont des portées différentes.

Lorsque vous décomposez un problème important en problèmes plus petits, même si ces problèmes (fonctions) ne sont utilisés / résolus qu'une seule fois, vous bénéficiez de plusieurs avantages:

  • Lisibilité. Personne ne peut lire une fonction monolithique et comprendre complètement ce qu’elle fait. Vous pouvez continuer à vous mentir ou le diviser en petits morceaux qui ont du sens.

  • Localité de référence. Il devient maintenant impossible de déclarer et d'utiliser une variable, puis de la conserver et de l'utiliser à nouveau 100 lignes plus tard. Ces fonctions ont différentes portées.

  • Essai. Bien qu'il soit seulement nécessaire de tester à l'unité les membres publics d'une classe, il peut également être souhaitable de tester certains membres privés. S'il existe une section critique d'une fonction longue susceptible de bénéficier d'un test, il est impossible de la tester indépendamment sans l'extraire dans une fonction distincte.

  • Modularité. Maintenant que vous avez des fonctions privées, vous pouvez en trouver une ou plusieurs qui pourraient être extraites dans une classe distincte, que celle-ci soit utilisée uniquement ici ou qu'elle soit réutilisable. Au point précédent, cette classe séparée sera probablement plus facile à tester, car elle nécessitera une interface publique.

L'idée de séparer les gros codes en éléments plus petits, plus faciles à comprendre et à tester, est un point clé du livre de Clean Code, de Oncle Bob . Au moment de la rédaction de cette réponse, le livre a neuf ans, mais il est tout aussi pertinent aujourd'hui qu'il l'était à l'époque.


la source
7
N'oubliez pas de maintenir des niveaux d'abstraction cohérents et des noms qui garantissent que vous ne ferez aucune surprise dans les méthodes Ne soyez pas surpris si un objet entier s'y cache.
candied_orange
14
Parfois, les méthodes de scission peuvent être bonnes, mais garder les choses en ligne peut aussi avoir des avantages. Si, par exemple, une vérification des limites peut être effectuée judicieusement à l'intérieur ou à l'extérieur d'un bloc de code, le maintien de ce bloc de code en ligne permettra de voir facilement si la vérification des limites est effectuée une fois, plus d'une fois ou pas du tout. . Introduire le bloc de code dans son propre sous-programme rendrait de telles choses plus difficiles à déterminer.
Supercat
7
La puce "lisibilité" pourrait être étiquetée "Abstraction". C'est à propos de l'abstraction. Les « petits morceaux» font une chose , une chose de bas niveau qui ne vous intéresse pas vraiment pour tous les détails de base lorsque vous lisez ou passez en revue la méthode publique. En résumant cette fonction à une fonction, vous pouvez la survoler et rester concentrée sur le grand schéma des choses / ce que la méthode publique fait à un niveau supérieur.
Mathieu Guindon
7
"Tester" la balle est douteuse à l’OMI. Si vos membres privés veulent être testés, ils appartiennent probablement à leur propre classe, en tant que membres publics. La première étape de l’extraction d’une classe / interface consiste à extraire une méthode.
Mathieu Guindon
6
Diviser une fonction uniquement par des numéros de ligne, des blocs de code et une portée variable sans se soucier de la fonctionnalité réelle est une idée terrible, et je dirais que la meilleure alternative à cela consiste à la laisser dans une fonction. Vous devriez séparer les fonctions en fonction des fonctionnalités. Voir la réponse de DaveGauer . Vous faites un peu allusion à cela dans votre réponse (avec des mentions de noms et de problèmes), mais je ne peux pas m'empêcher de penser que cet aspect nécessite davantage d'attention, compte tenu de son importance et de sa pertinence par rapport à la question.
Dukeling
36

C'est probablement une bonne idée!

Je ne suis pas d'accord avec la division de longues séquences d'actions linéaires en fonctions distinctes uniquement pour réduire la longueur moyenne de la fonction dans votre base de code:

function step1(){
  // ...
  step2(zarb, foo, biz);
}

function step2(zarb, foo, biz){
  // ...
  step3(zarb, foo, biz, gleep);
}

function step3(zarb, foo, biz, gleep){
  // ...
}

Vous avez maintenant ajouté des lignes de source et réduit considérablement la lisibilité totale. Surtout si vous passez maintenant beaucoup de paramètres entre chaque fonction pour garder trace de l'état. Beurk!

Toutefois , si vous avez réussi à extraire une ou plusieurs lignes dans une fonction pure qui remplit un seul objectif clair ( même si vous ne l'avez appelée qu'une fois ), vous avez amélioré la lisibilité:

function foo(){
  f = getFrambulation();
  g = deglorbFramb(f);
  r = reglorbulate(g);
}

Cela ne sera probablement pas facile dans des situations réelles, mais des fonctionnalités pures peuvent souvent être découvertes si vous y réfléchissez suffisamment longtemps.

Vous saurez que vous êtes sur la bonne voie lorsque vous avez des fonctions avec de beaux noms de verbe et que votre fonction parent les appelle et que tout se lit pratiquement comme un paragraphe de prose.

Puis, lorsque vous revenez des semaines plus tard pour ajouter plus de fonctionnalités et que vous constatez que vous pouvez réellement réutiliser une de ces fonctions, alors, oh, joie extatique! Quel délice radieux!

DaveGauer
la source
2
J'entends cet argument depuis de nombreuses années et il ne joue jamais dans le monde réel. Je parle spécifiquement de fonctions à une ou deux lignes, appelées depuis un seul endroit. Bien que l'auteur original pense toujours qu'il est "plus lisible", les éditeurs ultérieurs l'appellent souvent le code Ravioli. Cela dépend de la profondeur de l'appel, en général.
Frank Hileman
34
@FrankHileman Pour être juste, je n'ai jamais vu un développeur complimenter le code de son antécédent.
T. Sar
4
@TSar le développeur précédent est la source de tous les problèmes!
Frank Hileman
18
@TSar Je n'ai jamais même complimenté le développeur précédent lorsque j'étais le développeur précédent.
IllusiveBrian
3
La bonne chose à propos de la décomposition est que vous pouvez alors composer foo = compose(reglorbulate, deglorbFramb, getFrambulation);
:;
19

La réponse est que cela dépend fortement de la situation. @Snowman aborde les aspects positifs de la rupture d'une fonction publique importante, mais il est important de se rappeler qu'il peut aussi y avoir des effets négatifs, car vous vous inquiétez à juste titre.

  • De nombreuses fonctions privées avec des effets secondaires peuvent rendre le code difficile à lire et assez fragile. Cela est particulièrement vrai lorsque ces fonctions privées dépendent des effets secondaires les uns des autres. Évitez d’avoir des fonctions étroitement couplées.

  • Les abstractions sont des fuites . Bien qu’il soit agréable de prétendre comment une opération est effectuée ou comment les données sont stockées n’a pas d’importance, il existe des cas où cela est le cas et il est important de les identifier.

  • La sémantique et le contexte sont importants. Si vous ne saisissez pas clairement le rôle d'une fonction dans son nom, vous pouvez à nouveau réduire la lisibilité et accroître la fragilité de la fonction publique. Cela est particulièrement vrai lorsque vous commencez à créer des fonctions privées avec un grand nombre de paramètres d'entrée et de sortie. Et tandis que depuis la fonction publique, vous voyez quelles fonctions privées elle appelle, depuis la fonction privée, vous ne voyez pas comment les fonctions publiques l'appellent. Cela peut conduire à une "correction de bogue" dans une fonction privée qui casse la fonction publique.

  • Le code fortement décomposé est toujours plus clair pour l'auteur que pour les autres. Cela ne signifie pas que cela ne peut toujours pas être clair pour les autres, mais il est facile de dire que cela a un sens parfait que cela bar()doit être appelé avant foo()au moment de la rédaction.

  • Réutilisation dangereuse. Votre fonction publique a probablement limité les entrées possibles à chaque fonction privée. Si ces hypothèses d'entrée ne sont pas correctement capturées (il est difficile de documenter TOUTES les hypothèses), il est possible que quelqu'un réutilise incorrectement l'une de vos fonctions privées, en introduisant des bogues dans la base de code.

Les fonctions de décomposition sont basées sur leur cohésion interne et leur couplage, et non sur leur longueur absolue.

Dlasalle
la source
6
Ignorer la lisibilité, examinons l'aspect de rapport de bogue: si l'utilisateur signale qu'un bogue s'est produit MainMethod(assez facile à obtenir, les symboles sont généralement inclus et vous devriez avoir un fichier de déréférencement de symbole) qui est constitué de 2k lignes (les numéros de ligne ne sont jamais inclus dans rapports de bugs) et c'est un NRE qui peut arriver à 1k de ces lignes, eh bien je serai damné si j'examine cela. Mais s’ils me disent que cela s’est passé en SomeSubMethodA8 lignes et que l’exception était un NRE, alors je trouverai probablement une solution assez simple.
410_Gone
2

À mon humble avis, l'intérêt de retirer des blocs de code uniquement pour dissocier la complexité serait souvent lié à la différence de complexité entre:

  1. Une description complète et précise en langage humain de ce que fait le code, y compris de la façon dont il traite les cas critiques , et

  2. Le code lui-même.

Si le code est beaucoup plus compliqué que la description, le remplacement du code en ligne par un appel de fonction peut faciliter la compréhension du code environnant. Par ailleurs, certains concepts peuvent être exprimés de manière plus lisible dans un langage informatique que dans un langage humain. Je considérerais w=x+y+z;, par exemple, plus lisible que w=addThreeNumbersAssumingSumOfFirstTwoDoesntOverflow(x,y,z);.

Au fur et à mesure que se divise une fonction importante, il y aura de moins en moins de différence entre la complexité des sous-fonctions et leurs descriptions, et l'avantage des subdivisions ultérieures diminuera. Si les éléments sont divisés au point que les descriptions seraient plus compliquées que le code, des fractionnements supplémentaires aggraveraient le code.

supercat
la source
2
Le nom de votre fonction est trompeur. Il n'y a rien sur le w = x + y + z qui indique une sorte de contrôle de débordement, alors que le nom de la méthode semble impliquer autrement. Un meilleur nom pour votre fonction serait "addAll (x, y, z)", par exemple, ou même simplement "Sum (x, y, z)".
T. Sar
6
Comme ils parlent de méthodes privées , cette question ne fait peut-être pas référence à C. De toute façon, ce n'est pas ce que je veux dire - vous pouvez simplement appeler la même fonction "somme" sans avoir à vous attaquer à l'hypothèse de la signature de la méthode. Par votre logique, toute méthode récursive doit s'appeler "DoThisAssumingStackDoesntOverflow", par exemple. Il en va de même pour "FindSubstringAssumingStartingPointIsInsideBounds".
T. Sar
1
En d'autres termes, les hypothèses de base, quelles qu'elles soient (deux nombres ne débordent pas, la pile ne débordera pas, l'index a été passé correctement) ne devraient pas figurer sur le nom de la méthode. Ces choses devraient être documentées, bien sûr, si nécessaire, mais pas sur la signature de la méthode.
T. Sar
1
@TSar: J'étais un peu facétieux avec le nom, mais le point clé est que (passer à la moyenne parce que c'est un meilleur exemple), un programmeur qui voit "(x + y + z + 1) / 3" dans son contexte sera Bien mieux placé pour savoir s’il s’agit d’un moyen approprié de calculer la moyenne en fonction du code d’appel que celui qui examine la définition ou le site d’appel de int average3(int n1, int n2, int n3). Si on divisait la fonction "moyenne", quelqu'un qui voudrait savoir si elle répondait aux besoins devait regarder à deux endroits.
Supercat
1
Si nous prenons C # par exemple, vous n’auriez qu’une seule méthode "Moyenne", qui peut accepter un nombre quelconque de paramètres. En fait, en c #, cela fonctionne sur n'importe quelle collection de nombres - et je suis sûr que c'est beaucoup plus facile à comprendre que de s'attaquer aux mathématiques grossières dans le code.
T. Sar
2

C'est un défi d'équilibre.

Plus c'est mieux

La méthode privée fournit efficacement un nom pour le code contenu, et parfois une signature significative (sauf si la moitié de ses paramètres sont des données de tramp complètement ad-hoc avec des interdépendances peu claires et non documentées).

Donner des noms à des constructions de code est généralement bon, à condition que les noms suggèrent un contrat significatif pour l'appelant, et que le contrat de la méthode privée corresponde exactement à ce que le nom suggère.

En se forçant à penser à des contrats significatifs pour de plus petites parties du code, le développeur initial peut détecter certains bogues et les éviter sans douleur, même avant tout test de développement. Cela ne fonctionne que si le développeur aspire à une dénomination concise (sonorité simple mais précise) et souhaite adapter les limites de la méthode privée afin qu'une dénomination concise soit même possible.

La maintenance ultérieure est également facilitée, car un nommage supplémentaire aide le code à s'auto-documenter .

  • Contrats sur petits morceaux de code
  • Les méthodes de niveau supérieur se transforment parfois en une courte séquence d'appels, chacun effectuant quelque chose d'important et distinctement nommé - accompagné de la couche mince de traitement des erreurs générales et externes. Une telle méthode peut devenir une ressource de formation précieuse pour quiconque doit rapidement devenir un expert de la structure générale du module.

Jusqu'à ce que ça devienne trop fin

Est-il possible d'exagérer de donner des noms à de petits morceaux de code et de se retrouver avec trop de méthodes privées, trop petites? Sûr. Un ou plusieurs des symptômes suivants indiquent que les méthodes deviennent trop petites pour être utiles:

  • Trop de surcharges qui ne représentent pas vraiment des signatures alternatives pour la même logique essentielle, mais plutôt une seule pile d'appels fixes.
  • Synonymes utilisés juste pour faire référence au même concept à plusieurs reprises ("nommer divertissant")
  • Beaucoup de décorations de nommage superficielles telles que XxxInternalou DoXxx, surtout s'il n'y a pas de système unifié pour les introduire.
  • Les noms maladroits presque plus longs que l'implémentation elle-même, comme LogDiskSpaceConsumptionUnlessNoUpdateNeeded
Jirka Hanika
la source
1

Contrairement à ce que d'autres ont dit, je dirais qu'une méthode publique longue est une odeur de conception qui n'est pas corrigée par la décomposition en méthodes privées.

Mais parfois, j'ai une grande méthode publique qui pourrait être divisée en étapes plus petites

Si tel est le cas, je dirais que chaque étape devrait être un citoyen de première classe avec une responsabilité unique. Dans un paradigme orienté objet, je suggérerais de créer une interface et une implémentation pour chaque étape de manière à ce que chacune ait une responsabilité unique, facilement identifiable et puisse être nommée de manière à ce que la responsabilité soit claire. Cela vous permet de tester unitairement la méthode (autrefois) grande publique, ainsi que chaque étape, indépendamment les unes des autres. Ils devraient tous être documentés aussi bien.

Pourquoi ne pas décomposer en méthodes privées? Voici quelques raisons:

  • Couplage étroit et testabilité. En réduisant la taille de votre méthode publique, vous avez amélioré sa lisibilité, mais tout le code est toujours étroitement couplé. Vous pouvez tester à l'unité les méthodes privées individuelles (à l'aide des fonctionnalités avancées d'un framework de test), mais vous ne pouvez pas facilement tester la méthode publique indépendamment des méthodes privées. Cela va à l’encontre des principes des tests unitaires.
  • Taille et complexité de la classe. Vous avez réduit la complexité d'une méthode, mais vous avez augmenté la complexité de la classe. La méthode publique est plus facile à lire, mais la classe est maintenant plus difficile à lire car elle a plus de fonctions qui définissent son comportement. Ma préférence va aux petites classes à responsabilité unique. Une méthode longue indique donc que la classe en fait trop.
  • Ne peut pas être réutilisé facilement. Il est fréquent que la maturité du code permette la réutilisation est utile. Si vos étapes sont dans des méthodes privées, elles ne peuvent pas être réutilisées ailleurs sans les extraire au préalable. De plus, cela peut encourager le copier-coller lorsqu'une étape est nécessaire ailleurs.
  • Fractionner de cette manière risque d'être arbitraire. Je dirais que scinder une longue méthode publique ne prend pas autant de réflexion ou de conception que de scinder les responsabilités en classes. Chaque classe doit être justifiée par un nom propre, une documentation et des tests, alors qu'une méthode privée ne reçoit pas autant d'attention.
  • Cache le problème. Vous avez donc décidé de scinder votre méthode publique en petites méthodes privées. Maintenant, il n'y a pas de problème! Vous pouvez continuer à ajouter de plus en plus d'étapes en ajoutant de plus en plus de méthodes privées! Au contraire, je pense que c'est un problème majeur. Il établit un modèle pour ajouter de la complexité à une classe qui sera suivie de corrections de bogues et d'implémentations de fonctionnalités. Bientôt, vos méthodes privées vont grandir et il faudra les séparer.

mais je crains que forcer quiconque lisant la méthode à utiliser différentes méthodes privées ne nuise à la lisibilité

C'est un argument que j'ai eu avec l'un de mes collègues récemment. Il fait valoir qu'avoir le comportement complet d'un module dans le même fichier / la même méthode améliore la lisibilité. Je conviens que le code est plus facile à suivre lorsqu'il est ensemble, mais le code est moins facile à raisonner à mesure que la complexité augmente. À mesure que le système se développe, il devient difficile de raisonner sur le module dans son ensemble. Lorsque vous décomposez la logique complexe en plusieurs classes, chacune ayant une responsabilité unique, il devient alors beaucoup plus facile de raisonner chaque partie.

Samuel
la source
1
Je suis en désaccord. Une abstraction excessive ou prématurée conduit à un code inutilement complexe. Une méthode privée peut être la première étape d’un chemin qui pourrait éventuellement nécessiter une interface et une synthèse, mais votre approche suggère de flâner dans des endroits qui n’auront peut-être pas besoin de visites.
WillC
1
Je comprends d’où vous venez, mais je pense que le code a une odeur semblable à celle posée par OP, ce qui indique qu’il est prêt pour une meilleure conception. Une abstraction prématurée serait de concevoir ces interfaces avant même que vous ne rencontriez ce problème.
Samuel
Je suis d'accord avec cette approche. En fait, quand vous suivez TDD, c'est la progression naturelle. L'autre personne dit que c'est trop d'abstraction prématurée, mais je trouve qu'il est beaucoup plus facile de se moquer rapidement d'une fonctionnalité dont j'ai besoin dans une classe séparée (derrière une interface) que de devoir l'implémenter réellement dans une méthode privée pour que le test réussisse .
Eternal21