Mon patron me demande d'arrêter d'écrire de petites fonctions et de tout faire dans la même boucle

209

J'ai lu un livre intitulé Clean Code de Robert C. Martin. Dans ce livre, j'ai vu de nombreuses méthodes pour nettoyer le code, comme écrire de petites fonctions, choisir des noms avec soin, etc. Cela semble de loin le livre le plus intéressant sur le code épuré que j'ai lu. Cependant, aujourd’hui, mon patron n’a pas aimé la façon dont j’ai écrit le code après avoir lu ce livre.

Ses arguments étaient

  • Écrire de petites fonctions est une tâche pénible car cela vous oblige à vous déplacer dans chaque petite fonction pour voir ce que le code fait.
  • Mettez tout dans une grande boucle principale même si la boucle principale compte plus de 300 lignes, la lecture est plus rapide.
  • N'écrivez que de petites fonctions si vous devez dupliquer du code.
  • N'écrivez pas une fonction avec le nom du commentaire, mettez votre ligne de code complexe (3-4 lignes) avec un commentaire ci-dessus; De même, vous pouvez modifier le code défaillant directement

Ceci est contre tout ce que j'ai lu. Comment écrivez-vous habituellement le code? Une grande boucle principale, pas de petites fonctions?

Le langage que j'utilise est principalement le Javascript. J'ai vraiment du mal à lire car j'ai supprimé toutes mes petites fonctions clairement nommées et tout mis en boucle. Cependant, mon patron aime ça de cette façon.

Un exemple était:

// The way I would write it
if (isApplicationInProduction(headers)) {
  phoneNumber = headers.resourceId;
} else {
  phoneNumber = DEV_PHONE_NUMBER;
}

function isApplicationInProduction(headers) {
   return _.has(headers, 'resourceId');
}

// The way he would write it
// Take the right resourceId if application is in production
phoneNumber = headers.resourceId ? headers.resourceId : DEV_PHONE_NUMBER;

Dans le livre que j'ai lu, par exemple, les commentaires sont considérés comme des échecs lors de l'écriture de code propre, car ils sont obsolètes si vous écrivez de petites fonctions et conduisent souvent à des commentaires non mis à jour (vous modifiez votre code et non le commentaire). Cependant, ce que je fais est de supprimer le commentaire et d'écrire une fonction avec le nom du commentaire.

Eh bien, je voudrais quelques conseils, quelle méthode / pratique est préférable pour écrire du code propre?

GitCommit Victor B.
la source
17
Juste pour que vous sachiez, John Carmack serait probablement d'accord avec votre patron .
Walen
4
phoneNumber = headers.resourceId?: DEV_PHONE_NUMBER;
Josué
10
Validez que vous voulez travailler sur place, où la direction vous a dit COMMENT faire votre travail, au lieu de CE QUI a besoin d'être résolu.
Konstantin Petrukhnov
8
@rjmunro À moins que vous n'aimiez vraiment votre travail, gardez à l'esprit qu'il y a moins de développeurs que d'emplois. Donc, pour citer Martin Fowler: "Si vous ne pouvez pas changer d’organisation, changez d’organisation!" et les patrons me disant comment coder sont quelque chose que je vous conseille de changer.
Niels van Reijmersdal
10
Ne jamais avoir une isApplicationInProduction()fonction! Vous devez avoir des tests, et les tests sont inutiles si votre code se comporte différemment de celui de sa production. C'est comme avoir intentionnellement du code non testé / non couvert en production: cela n'a aucun sens.
Ronan Paixão

Réponses:

215

Prenons d'abord les exemples de code. Vous favorisez:

if (isApplicationInProduction(headers)) {
  phoneNumber = headers.resourceId;
} else {
  phoneNumber = DEV_PHONE_NUMBER;
}

function isApplicationInProduction(headers) {
   return _.has(headers, 'resourceId');
}

Et votre patron l'écrirait ainsi:

// Take the right resourceId if application is in production
phoneNumber = headers.resourceId ? headers.resourceId : DEV_PHONE_NUMBER;

À mon avis, les deux ont des problèmes. En lisant votre code, je me suis immédiatement dit: "vous pouvez le remplacer ifpar une expression ternaire". Ensuite, j'ai lu le code de votre patron et j'ai pensé: "Pourquoi a-t-il remplacé votre fonction par un commentaire?".

Je suggère que le code optimal est entre les deux:

phoneNumber = isApplicationInProduction(headers) ? headers.resourceId : DEV_PHONE_NUMBER;

function isApplicationInProduction(headers) {
   return _.has(headers, 'resourceId');
}

Cela vous donne le meilleur des deux mondes: une expression de test simplifiée et le commentaire est remplacé par du code testable.

Cependant, en ce qui concerne la conception de code par votre patron:

Écrire de petites fonctions est une tâche pénible, car il vous oblige à vous déplacer dans chaque petite fonction pour voir ce que le code fait.

Si la fonction est bien nommée, ce n'est pas le cas. isApplicationInProductionCela va de soi et il ne devrait pas être nécessaire d'examiner le code pour voir ce qu'il fait. En fait, le contraire est vrai: l’examen du code révèle moins quant à l’intention que le nom de la fonction (c’est pourquoi votre patron doit recourir à des commentaires).

Mettez tout dans une grande boucle principale même si la boucle principale compte plus de 300 lignes, la lecture est plus rapide

La numérisation est peut-être plus rapide, mais pour vraiment "lire" le code, vous devez être capable de l'exécuter efficacement dans votre tête. C'est facile avec de petites fonctions et c'est vraiment très dur avec des méthodes de plusieurs centaines de lignes.

N'écrivez que de petites fonctions si vous devez dupliquer le code

Je ne suis pas d'accord. Comme votre exemple de code le montre, de petites fonctions bien nommées améliorent la lisibilité du code et doivent être utilisées chaque fois que vous n'êtes pas intéressé par le "comment", mais uniquement par le "quoi" d'une fonctionnalité.

N'écrivez pas une fonction avec le nom du commentaire, mettez votre ligne de code complexe (3-4 lignes) avec un commentaire ci-dessus. Comme cela, vous pouvez modifier le code défaillant directement

Je ne peux vraiment pas comprendre le raisonnement derrière celui-ci, en supposant que ce soit vraiment sérieux. C'est le genre de chose que je m'attendrais à voir écrite en parodie par le compte twitter de Expert Débutant . Les commentaires ont un défaut fondamental: ils ne sont pas compilés / interprétés et ne peuvent donc pas être testés à l'unité. Le code est modifié et le commentaire est laissé seul et vous finissez par ne pas savoir qui est correct.

L'écriture de code auto-documenté est difficile et nécessite parfois des documents supplémentaires (même sous forme de commentaires). Mais le point de vue de "Uncle Bob" selon lequel les commentaires sont un échec de codage est trop souvent vrai.

Demandez à votre patron de lire le livre de code propre et essayez de résister à rendre votre code moins lisible, juste pour le satisfaire. En fin de compte, si vous ne pouvez pas le persuader de changer, vous devez soit vous aligner, soit trouver un nouveau patron capable de mieux coder.

David Arno
la source
26
Les petites fonctions sont facilement testées
Mawg
13
Quoth @ ExpertBeginner1 : «Je suis fatigué de voir des tonnes de petites méthodes partout dans notre code, alors à partir de maintenant, il y a un minimum de 15 lettres de crédit pour toutes les méthodes.»
Greg Bacon
34
"Les commentaires ont un défaut fondamental: ils ne sont pas compilés / interprétés et ne peuvent donc pas être testés à l'unité" En jouant ici l'avocat du diable, c'est également vrai si vous remplacez "commentaires" par "noms de fonctions".
mattecapu
11
@mattecapu, je vais prendre votre plaidoyer et le doubler immédiatement. N'importe quel vieux développeur de détritus peut commenter dans un commentaire en essayant de décrire le comportement d'un morceau de code. Décrire succinctement cette partie de code avec un nom de fonction correct requiert un communicateur habile. Les meilleurs développeurs sont des communicateurs qualifiés, car l'écriture de code concerne principalement la communication avec les autres développeurs et le compilateur comme préoccupation secondaire. Ergo, un bon développeur utilisera des fonctions bien nommées et aucun commentaire; Les développeurs médiocres cachent leurs faibles compétences derrière des excuses pour utiliser des commentaires.
David Arno
4
@DavidArno Toutes les fonctions ont des conditions préalables et postérieures, la question est de savoir si vous les documentez ou non. Si ma fonction prend un paramètre, qui est une distance en pieds mesurés, vous devez le fournir en pieds et non en kilomètres. C'est une condition préalable.
Jørgen Fogh
223

Il y a d'autres problèmes

Aucun des deux codes n'est bon, parce que les deux compliquent fondamentalement le code avec un scénario de test de débogage . Et si vous voulez tester plus de choses pour une raison quelconque?

phoneNumber = DEV_PHONE_NUMBER_WHICH_CAUSED_PROBLEMS_FOR_CUSTOMERS;

ou

phoneNumber = DEV_PHONE_NUMBER_FROM_OTHER_COUNTRY;

Voulez-vous ajouter encore plus de branches?

Le problème important est que vous dupliquez une partie de votre code et que vous ne testez donc pas le code réel. Vous écrivez du code de débogage pour tester le code de débogage, mais pas le code de production. C'est comme créer partiellement une base de code parallèle.

Vous vous disputez avec votre patron sur la manière d'écrire plus intelligemment un mauvais code. Au lieu de cela, vous devriez résoudre le problème inhérent au code lui-même.

Injection de dépendance

Voici à quoi devrait ressembler votre code:

phoneNumber = headers.resourceId;

Il n'y a pas de branchement ici, parce que la logique n'a pas de branchement. Le programme doit extraire le numéro de téléphone de l'en-tête. Période.

Si vous voulez avoir DEV_PHONE_NUMBER_FROM_OTHER_COUNTRYun résultat, vous devriez le mettre dans headers.resourceId. Une façon de le faire est simplement d’injecter un headersobjet différent pour les cas de test (désolé si ce n’est pas un code correct, mes compétences en JavaScript sont un peu rouillées):

function foo(headers){
    phoneNumber = headers.resourceId;
}

// Creating the test case
foo({resourceId: DEV_PHONE_NUMBER_FROM_OTHER_COUNTRY});

En supposant que cela headersfasse partie d'une réponse que vous recevez d'un serveur: idéalement, vous avez un serveur de test complet qui fournit headersdivers types à des fins de test. De cette façon, vous testez le code de production réel tel quel et non un code à moitié dupliqué qui pourrait ou non fonctionner comme le code de production.

nul
la source
11
J'ai envisagé d'aborder ce sujet dans ma propre réponse, mais je pensais que c'était déjà assez long. Alors +1 à toi pour le faire :)
David Arno
5
@DavidArno J'étais sur le point de l'ajouter en tant que commentaire à votre réponse, car la question était toujours verrouillée lorsque je l'ai lue pour la première fois, et à ma grande surprise, elle a été ouverte à nouveau et l'a ajouté comme réponse. Peut-être faudrait-il ajouter qu'il existe des dizaines de cadres / outils pour effectuer des tests automatisés. Surtout chez JS, il semble y en avoir un nouveau tous les jours. Cela pourrait être difficile à vendre au patron cependant.
null
56
@ DavidArno Vous auriez peut-être dû diviser votre réponse en réponses plus petites. ;)
krillgar
2
@ user949300 L'utilisation d'un OU au niveau du bit ne serait pas sage;)
curieuxdannii
1
@curiousdannii Ouais, remarqué qu'il était trop tard pour éditer ...
user949300
59

Il n'y a pas de "bonne" ou de "mauvaise" réponse à cela. Cependant, je donnerai mon opinion sur 36 ans d'expérience professionnelle dans la conception et le développement de systèmes logiciels ...

  1. Il n’existe pas de "code auto-documenté". Pourquoi? Parce que cette affirmation est totalement subjective.
  2. Les commentaires ne sont jamais des échecs. Ce qui est un échec est un code qui ne peut être compris du tout sans commentaires.
  3. 300 lignes de code ininterrompues dans un bloc de code est un cauchemar de maintenance très sujet aux erreurs. Un tel bloc est fortement révélateur d'une mauvaise conception et planification.

En vous référant directement à l'exemple que vous avez fourni ... Il isApplicationInProduction()est plus intelligent (ou plus) de mettre en place sa propre routine. Aujourd'hui, ce test est simplement une vérification des "en-têtes" et peut être géré par un ?:opérateur ternaire ( ). Demain, le test sera peut-être beaucoup plus complexe. En outre, "headers.resourceId" n'a pas de relation claire avec le statut "en production" de l'application; Je soutiendrais qu'un test pour un tel statut doit être découplé des données sous-jacentes; un sous-programme le fera et un ternaire ne le fera pas. De plus, un commentaire utile expliquerait pourquoi resourceId est un test de "en production".

Veillez à ne pas abuser de "petites fonctions clairement nommées". Une routine devrait davantage résumer une idée que "juste du code". Je soutiens la suggestion de Amon phoneNumber = getPhoneNumber(headers)et ajoute que getPhoneNumber()devrait faire le test de "statut de production" avecisApplicationInProduction()

LiamF
la source
25
Il existe de bons commentaires qui ne sont pas un échec. Cependant, les commentaires qui sont presque textuellement le code qu’ils expliquent ou qui sont simplement des blocs de commentaires vides précédant une méthode / classe / etc. sont définitivement un échec.
jpmc26
3
Il est possible d'avoir un code plus petit et plus facile à lire que n'importe quelle description en anglais de ce qu'il fait et des coins qu'il ne gère pas. En outre, si une fonction est extraite de sa propre méthode, une personne qui lit la fonction ne saura pas quels cas d'angle sont ou ne sont pas gérés par ses appelants et, à moins que le nom de la fonction soit très prolixe, une personne examinant les appelants peut ne pas savoir quel coin. les cas sont traités par la fonction.
Supercat
7
Les commentaires ne sont jamais intrinsèquement des échecs. Les commentaires peuvent être des échecs, et le sont quand ils sont inexacts. Un code erroné peut être détecté à plusieurs niveaux, notamment par un comportement incorrect en mode boîte noire. Les commentaires erronés ne peuvent être détectés que par la compréhension humaine en mode boîte blanche, en reconnaissant que deux modèles sont décrits et que l'un d'eux est incorrect.
Timbo
7
@ Timbo Vous voulez dire, "... au moins l' un d'entre eux est incorrect." ;)
jpmc26
5
@immibis Si vous ne pouvez pas comprendre ce que le code fait sans commentaires, alors le code n'est probablement pas assez clair. L'objectif principal des commentaires est de clarifier pourquoi le code fait ce qu'il fait. C'est le codeur qui explique sa conception aux futurs responsables. Le code ne peut jamais fournir ce genre d'explication, aussi les commentaires comblent-ils ces lacunes de compréhension.
Graham
47

"Les entités ne doivent pas être multipliées au-delà de la nécessité."

- Le rasoir d'Occam

Le code doit être aussi simple que possible. Les insectes aiment se cacher dans la complexité, car ils sont difficiles à détecter. Alors, qu'est-ce qui rend le code simple?

Les petites unités (fichiers, fonctions, classes) sont une bonne idée . Les petites unités sont faciles à comprendre car il y a moins de choses à comprendre en même temps. Les humains normaux ne peuvent jongler qu'avec ~ 7 concepts à la fois. Mais la taille ne se mesure pas seulement en lignes de code . Je peux écrire le moins de code possible en «jouant» le code (choisir des noms de variable courts, utiliser des raccourcis «intelligents», écraser autant de code que possible sur une seule ligne), mais le résultat final n'est pas simple. Essayer de comprendre un tel code s'apparente davantage à de l'ingénierie inverse qu'à de la lecture.

Une façon de raccourcir une fonction consiste à extraire diverses fonctions d’aide. Cela peut être une bonne idée quand il extrait une pièce de complexité autonome . Pris isolément, ce morceau de complexité est beaucoup plus simple à gérer (et à tester!) Que lorsqu'il est intégré à un problème sans rapport.

Mais chaque appel de fonction entraîne une surcharge cognitive : je ne dois pas seulement comprendre le code dans mon code actuel, je dois aussi comprendre comment il interagit avec le code à l' extérieur . Je pense qu'il est juste de dire que la fonction que vous avez extraite introduit plus de complexité dans la fonction qu'elle n'en extrait . C'est ce que votre patron entendait par « petites fonctions [sont] une douleur, car il vous oblige à passer à chaque petite fonction pour voir ce que fait le code.

Parfois, des fonctions longues et ennuyeuses peuvent être incroyablement simples à comprendre, même si elles comportent des centaines de lignes. Cela a tendance à se produire dans le code d'initialisation et de configuration, par exemple lors de la création manuelle d'une interface graphique sans éditeur à glisser. Il n'y a pas de complexité inhérente que vous puissiez extraire de manière raisonnable. Mais si le formatage est lisible et qu'il y a des commentaires, il n’est vraiment pas difficile de suivre ce qui se passe.

Il existe de nombreux autres paramètres de complexité: Le nombre de variables dans une étendue doit être aussi petit que possible. Cela ne signifie pas que nous devrions éviter les variables. Cela signifie que nous devrions limiter chaque variable à la plus petite portée possible. Les variables deviennent également plus simples si nous ne changeons jamais la valeur qu'elles contiennent.

Une métrique très importante est la complexité cyclomatique (complexité de McCabe). Il mesure le nombre de chemins indépendants dans un morceau de code. Ce nombre augmente de manière exponentielle avec chaque condition. Chaque condition ou boucle double le nombre de chemins. Des preuves suggèrent qu'un score de plus de 10 points est trop complexe. Cela signifie qu'une fonction très longue ayant peut-être un score de 5 est peut-être préférable à une fonction très courte et dense avec un score de 25. Nous pouvons réduire la complexité en extrayant le flux de contrôle dans des fonctions séparées.

Votre conditionnel est un exemple de complexité pouvant être entièrement extraite:

function bigFatFunction(...) {
  ...
  phoneNumber = getPhoneNumber(headers);
  ...
}

...

function getPhoneNumber(headers) {
  return headers.resourceId ? headers.resourceId : DEV_PHONE_NUMBER;
}

C'est encore très sur le point d'être utile. Je ne suis pas sûr que cela diminue considérablement la complexité, car ce conditionnel n'est pas très conditionnel . En production, il faudra toujours suivre le même chemin.


La complexité ne peut jamais disparaître. Il ne peut être que mélangé. Est-ce que beaucoup de petites choses sont plus simples que quelques grandes? Cela dépend beaucoup des circonstances. Habituellement, il y a une combinaison qui se sent juste bien. Trouver un compromis entre différents facteurs de complexité demande de l'intuition, de l'expérience et un peu de chance.

Savoir écrire de très petites fonctions et des fonctions très simples est une compétence utile, car vous ne pouvez pas choisir sans connaître les alternatives. Suivre aveuglément les règles ou les meilleures pratiques sans se préoccuper de la façon dont elles s’appliquent à la situation actuelle donne au mieux des résultats moyens et au pire des programmes de culte du fret .

C'est là où je ne suis pas d'accord avec votre patron. Ses arguments ne sont pas invalides, mais le livre de code propre n'est pas faux non plus. Il est probablement préférable de suivre les directives de votre patron, mais le fait même que vous réfléchissiez à ces problèmes, essayant de trouver une meilleure solution, est très prometteur. À mesure que vous gagnerez de l'expérience, il vous sera plus facile de trouver un bon affacturage pour votre code.

(Remarque: cette réponse est basée en partie sur des réflexions tirées du billet de blog de Reasonable Code sur The Whiteboard de Jimmy Hoffa , qui fournit une vue de haut niveau de ce qui rend le code simple.)

Amon
la source
Je suis général, j'ai aimé votre réponse. Cependant, je suis en désaccord avec la mesure de complexité cyclomatic de mcabes. D'après ce que j'en ai vu, cela ne présente pas une véritable mesure de la complexité.
Robert Baron
27

Le style de programmation de Robert Martin est polarisant. Vous trouverez de nombreux programmeurs, même expérimentés, qui trouvent de nombreuses excuses pour expliquer la division "à ce point", c'est trop, et pourquoi garder les fonctions un peu plus grandes est "la meilleure façon". Cependant, la plupart de ces "arguments" sont souvent l'expression d'une réticence à changer de vieilles habitudes et à apprendre quelque chose de nouveau.

Ne les écoute pas!

Chaque fois que vous pouvez enregistrer un commentaire en refacturant un morceau de code dans une fonction distincte avec un nom expressif, faites-le - cela améliorera probablement votre code. Vous n’êtes pas allé aussi loin que Bob Martin le fait dans son code propre, mais la grande majorité du code que j’ai vu dans le passé et qui posait des problèmes de maintenance contenait des fonctions trop volumineuses, pas trop petites. Donc, vous devriez essayer d’écrire des fonctions plus petites avec des noms auto-descriptifs.

Les outils de refactoring automatiques facilitent, simplifient et sécurisent l'extraction de méthodes. Et s'il vous plaît, ne prenez pas au sérieux les personnes qui recommandent d'écrire des fonctions avec> 300 lignes - ces personnes ne sont certainement pas qualifiées pour vous dire comment coder.

Doc Brown
la source
53
"Ne les écoute pas!" : étant donné que son patron demande à l'OP de cesser de diviser le code, l'OP devrait probablement éviter votre conseil. Même si le patron ne veut pas changer ses vieilles habitudes. Notez également que, comme indiqué dans les réponses précédentes, le code de l'OP et le code de son patron sont mal écrits, et vous ne le mentionnez pas (intentionnellement ou non) dans votre réponse.
Arseni Mourzenko
10
@ArseniMourzenko: nous ne sommes pas tous obligés de nous boucler devant son patron. J'espère que le PO est assez vieux pour savoir quand il doit faire ce qu'il faut ou quand il doit faire ce que son patron dit. Et oui, je n'allais pas entrer dans les détails de l'exemple intentionnellement, il y a déjà suffisamment de réponses pour discuter de ces détails.
Doc Brown
8
@DocBrown D'accord. 300 lignes est discutable pour toute une classe. Une fonction de 300 lignes est obscène.
JimmyJames
30
J'ai vu beaucoup de classes de plus de 300 lignes qui sont parfaitement bonnes. Java est si bavard que vous ne pouvez presque rien faire de significatif dans une classe sans autant de code. Donc, "le nombre de lignes de code dans une classe", en soi, n'est pas une métrique significative, pas plus que nous ne considérerions le SLOC comme une métrique significative pour la productivité du programmeur.
Robert Harvey
9
De plus, j'ai vu le sage conseil d'oncle Bob mal interprété et abusé à tel point que j'ai des doutes qu'il soit utile à quiconque sauf aux programmeurs expérimentés .
Robert Harvey
23

Dans votre cas: vous voulez un numéro de téléphone. Soit il est évident que vous obtiendrez un numéro de téléphone, puis vous écrivez le code évident. Ou bien, comment obtenir un numéro de téléphone n'est pas évident, alors vous écrivez une méthode.

Dans votre cas, comment obtenir le numéro de téléphone n’est pas évident, vous devez donc écrire une méthode. L’implémentation n’est pas évidente, mais c’est pourquoi vous l’intégrez dans une méthode distincte de sorte que vous ne devez la traiter qu’une seule fois. Un commentaire serait utile car l'implémentation n'est pas évidente.

La méthode "isApplicationInProduction" est assez inutile. L'appeler depuis votre méthode getPhonenumber ne rend pas plus évident l'implémentation, mais rend plus difficile la compréhension de ce qui se passe.

N'écris pas de petites fonctions. Écrivez des fonctions qui ont un but bien défini et qui répondent à ce but bien défini.

PS Je n'aime pas du tout la mise en œuvre. Cela suppose que l'absence de numéro de téléphone signifie qu'il s'agit d'une version dev. Donc, si le numéro de téléphone est absent de la production, non seulement vous ne le gérez pas, mais vous le remplacez par un numéro de téléphone aléatoire. Imaginez que vous avez 10 000 clients et que 17 n’ont pas de numéro de téléphone et que la production pose problème. Que vous soyez en production ou en développement doit être vérifié directement, et non dérivé d’autre chose.

gnasher729
la source
1
"N'écrivez pas de petites fonctions. Écrivez des fonctions qui ont un but bien défini et qui répondent à ce but bien défini." c'est le critère correct pour la scission du code. si une fonction en fait trop (comme plus d'un), des fonctions disparates , puis séparez-la. Le principe de responsabilité unique est le principe directeur.
robert bristow-johnson
16

Même en ignorant le fait qu'aucune des deux implémentations n'est vraiment bonne, je ferais remarquer qu'il s'agit essentiellement d'une question de goût, du moins au niveau de l'abstraction de fonctions triviales à usage unique.

Le nombre de lignes n'est pas une métrique utile dans la plupart des cas.

300 (voire 3000) lignes de code purement séquentiel totalement trivial (programme d'installation ou quelque chose du genre) est rarement un problème (mais peut être mieux généré automatiquement ou sous forme de tableau de données ou quelque chose comme ça), 100 lignes de boucles imbriquées avec beaucoup de choses compliquées. les conditions de sortie et les maths que vous pourriez trouver dans l’élimination gaussienne ou l’inversion matricielle ou autre peuvent être beaucoup trop difficiles à suivre facilement.

Pour moi, je n’écrirais pas une fonction à usage unique à moins que la quantité de code nécessaire pour déclarer la chose soit beaucoup plus petite que la quantité de code constituant l’implémentation (sauf si j’avais des raisons comme vouloir vouloir pouvoir facilement faire une injection de fautes). Une seule condition correspond rarement à ce projet de loi.

Maintenant, je viens d'un petit monde intégré, dans lequel nous devons également tenir compte d'éléments tels que la profondeur de pile et les frais généraux d'appel / de retour (ce qui contredit encore une fois le genre de fonctions minuscules qui semblent être préconisées ici), ce qui pourrait biaiser ma conception. décisions, mais si je voyais cette fonction originale dans une révision du code, cela donnerait une ancienne flamme usenet en réponse.

Le goût est design est difficile à enseigner et ne vient vraiment qu'avec l'expérience, je ne suis pas sûr qu'il puisse être réduit à des règles concernant la longueur des fonctions, et même la complexité cyclomatique a ses limites en tant que métrique (parfois, les choses sont simplement compliquées, mais vous les abordez).
Cela ne veut pas dire que le code propre ne traite pas de certaines choses intéressantes, mais qu'il faut en tenir compte, mais que la coutume locale et le rôle de la base de code existante doivent également être pris en compte.

Cet exemple particulier me semble aller dans les moindres détails, je serais plus préoccupé par des problèmes de niveau beaucoup plus élevé car cela est beaucoup plus important pour la capacité de comprendre et de déboguer un système facilement.

Dan Mills
la source
1
Je suis tout à fait d’accord - il me faudrait une ligne très complexe pour envisager de l’envelopper dans une fonction ... Je ne voudrais certainement pas envelopper une ligne de valeur ternaire / par défaut. J'ai emballé des doublures, mais en général, il s'agit des scripts de shell dans lesquels dix tuyaux permettent d'analyser quelque chose et le code est incompréhensible sans l'exécuter.
TemporalWolf
15

Ne mettez pas tout en un, mais ne le faites pas trop souvent non plus:

function isApplicationInProduction(headers) {
   return _.has(headers, 'resourceId');
}

Le problème avec la grande boucle est qu’il est vraiment difficile de voir sa structure globale quand elle couvre plusieurs écrans. Essayez donc de supprimer de gros morceaux, idéalement des morceaux qui ont une responsabilité unique et qui sont réutilisables.

Le problème avec la fonction minuscule ci-dessus est que, bien que l'atomicité et la modularité soient généralement bonnes, cela peut aller trop loin. Si vous n'allez pas réutiliser la fonction ci-dessus, cela nuit à la lisibilité et à la maintenabilité du code. Pour approfondir les détails, vous devez accéder à la fonction au lieu de pouvoir lire le détail en ligne, et l'appel de la fonction occupe à peine moins d'espace que le détail lui-même.

De toute évidence, il faut trouver un équilibre entre les méthodes qui en font trop et les méthodes qui en font trop peu . Je n’écarterais jamais une fonction minuscule comme ci-dessus à moins que l’appel ne soit appelé de plusieurs endroits différents. Même dans ce cas, j’y réfléchissais à deux fois, car la fonction n’était tout simplement pas aussi substantielle pour ce qui est d’introduire une nouvelle logique. une telle peine justifie d'avoir sa propre existence.

Brad Thomas
la source
2
Je comprends qu'un seul doublure booléenne est facile à lire, mais cela seul ne permet vraiment que d'expliquer ce qui se passe. J'écris toujours des fonctions qui enveloppent des expressions ternaires simples, car le nom de la fonction aide à expliquer la raison "Pourquoi", j'effectue cette vérification de condition. Ceci est particulièrement utile lorsque quelqu'un de nouveau (ou vous-même dans 6 mois) doit comprendre la logique métier.
AJ X.
14

Il semble que ce que vous voulez réellement est la suivante:

phoneNumber = headers.resourceId || DEV_PHONE_NUMBER

Cela devrait être explicite pour quiconque le lit: définissez- phoneNumberle sur resourceIds'il est disponible, ou par défaut sur DEV_PHONE_NUMBERsi ce n'est pas le cas.

Si vous voulez vraiment définir cette variable uniquement en production, vous devriez avoir une autre méthode utilitaire, plus canonique, d'application à l'échelle de l'application (qui ne nécessite pas de paramètres) pour déterminer votre emplacement d'origine. Lire les en-têtes pour cette information n'a pas de sens.

BlueRaja - Danny Pflughoeft
la source
Cela va de soi ce que ça fait (avec un peu de devinette quelle langue vous utilisez), mais ce n'est pas du tout évident de ce qui se passe. Apparemment, le développeur suppose que phoneNumber est stocké sous "resourceId" dans la version de production et que resourceId n'est pas présent dans la version de développement, et il souhaite utiliser DEV_PHONE_NUMBER dans la version de développement. Ce qui signifie que le numéro de téléphone est stocké sous un nom étrange. titre, et cela signifie que les choses iront très mal si un numéro de téléphone manque dans la version de production.
gnasher729
14

Permettez-moi d'être franc: il me semble que votre environnement (langage / cadre / conception de classe, etc.) n'est pas vraiment adapté au code "propre". Vous mélangez toutes sortes de choses possibles dans quelques lignes de code qui ne devraient pas vraiment être proches les unes des autres. Quelle activité une fonction a-t-elle avec sachant que cela resourceId==undefsignifie que vous n'êtes pas en production, que vous utilisez un numéro de téléphone par défaut dans des systèmes autres que de production, que le resourceId est enregistré dans des "en-têtes" et ainsi de suite? Je suppose que ce headerssont des en-têtes HTTP, de sorte que vous laissez même la décision concernant l'environnement dans lequel vous vous trouvez, au profit de l'utilisateur final?

Le fait de factoriser des éléments isolés dans des fonctions ne vous aidera pas beaucoup à résoudre ce problème sous-jacent.

Quelques mots-clés à rechercher:

  • découplage
  • cohésion
  • injection de dépendance

Vous pouvez obtenir ce que vous voulez (dans d'autres contextes) avec zéro ligne de code, en déplaçant les responsabilités du code et en utilisant des cadres modernes (qui peuvent ou non exister pour votre environnement / langage de programmation).

D'après votre description ("300 lignes de code dans une fonction 'principale'"), même le mot "fonction" (au lieu de méthode) me laisse supposer qu'il n'y a pas de but à ce que vous essayez d'atteindre. Dans cet environnement de programmation de la vieille école (c.-à-d. Une programmation impérative de base avec peu de structure, certainement pas de classes significatives, pas de modèle de cadre de classe comme MVC ou autre), il n'y a vraiment aucune raison de faire quoi que ce soit . Vous ne sortirez jamais du puits sans changements fondamentaux. Au moins, votre patron semble vous permettre de créer des fonctions pour éviter la duplication de code, c’est une bonne première étape!

Je connais très bien le type de code et le type de programmeur que vous décrivez. Franchement, s'il s'agissait d'un collègue, mon conseil serait différent. Mais comme c'est votre patron, il est inutile que vous alliez vous battre pour ça. Non seulement votre patron peut vous ignorer, mais vos ajouts de code conduiront en fait à un code pire si seulement vous exécutiez «votre affaire» partiellement, et que votre patron (et probablement d’autres personnes) continue de fonctionner comme avant. Le résultat final sera peut-être simplement meilleur si vous vous adaptez à leur style de programmation (bien sûr en travaillant sur cette base de code particulière) et essayez, dans ce contexte, de tirer le meilleur parti de celle-ci.

AnoE
la source
1
Je conviens à 100% qu'il y a ici des composants implicites qui doivent être séparés, mais sans en savoir plus sur le langage / cadre, il est difficile de savoir si une approche orientée objet a du sens. Le découplage et le principe de responsabilité unique sont importants dans toutes les langues, de la fonction pure (par exemple Haskell) à l’impératif pur (par exemple C.). Ma première étape - si le patron le permet - serait de convertir la fonction principale en une fonction de répartition ( comme un plan ou une table des matières) qui se lirait dans un style déclaratif (décrivant des stratégies, et non des algorithmes) et chargeant le travail sur d’autres fonctions.
David Leppik
JavaScript est un prototype, avec des fonctions de première classe. C'est par nature OO, mais pas au sens classique du terme, vos hypothèses risquent donc de ne pas être correctes. Quelques heures à regarder des vidéos de Crockford sur YouTube ...
Kevin_Kinsey
13

"Nettoyer" est l'un des objectifs de l'écriture de code. Ce n'est pas le seul objectif. Un autre objectif louable est la colocalité . En termes informels, la colocalité signifie que les personnes qui tentent de comprendre votre code n'ont pas besoin de sauter partout pour voir ce que vous faites. Utiliser une fonction bien nommée au lieu d'une expression ternaire peut sembler une bonne chose, mais selon le nombre de ces fonctions et leur emplacement, cette pratique peut devenir une nuisance. Je ne peux pas vous dire si vous avez dépassé cette limite, sauf pour dire que si des personnes se plaignent, vous devez écouter, surtout si ces personnes ont leur mot à dire sur votre situation d'emploi.

utilisateur1172763
la source
2
"... sauf pour dire que si des personnes se plaignent, vous devez les écouter, surtout si elles ont leur mot à dire sur votre situation professionnelle". OMI c'est vraiment un mauvais conseil. Sauf si vous êtes un développeur gravement pauvre qui doit apprécier tout travail que vous pouvez obtenir, appliquez toujours le principe "si vous ne pouvez pas changer de travail, changez de travail". Ne vous sentez jamais redevable à une entreprise; ils ont besoin de vous plus que vous n'en avez besoin, alors éloignez-vous d'un meilleur endroit s'ils n'offrent pas ce que vous voulez.
David Arno
4
J'ai un peu bougé pendant ma carrière. Je ne pense pas avoir jamais occupé un poste où je voyais à 100% mon chef se disputer de la manière de coder. Nous sommes des êtres humains avec nos propres origines et philosophies. Donc, personnellement, je ne quitterais pas mon travail simplement parce qu'il y avait quelques normes de codage que je n'aimais pas. (Les conventions de nommage qui plient les doigts semblent plaire aux gestionnaires sont particulièrement contraires à la façon dont je coderais si je le laissais faire moi-même.) Mais vous avez raison, un bon programmeur ne devrait pas avoir trop à se soucier de rester employé. .
user1172763
6

L'utilisation de petites fonctions est, en général, une bonne pratique. Mais idéalement, j’estime que l’introduction d’une fonction doit séparer les gros blocs logiques ou réduire la taille globale du code en le séchant. L'exemple que vous avez donné à la fois rallonge le code et demande plus de temps à un développeur pour lire, tandis que l'alternative courte n'explique pas que la "resourceId"valeur est uniquement présente en production. Quelque chose de simple comme celui-là est à la fois facile à oublier et rassurant lorsque vous essayez de le travailler, en particulier si vous êtes encore nouveau dans le code.

Je ne dirai pas que vous devriez absolument utiliser un ternaire, certaines personnes avec qui j'ai travaillé préfèrent un peu plus longtemps if () {...} else {...}, c'est surtout un choix personnel. J'ai tendance à préférer une approche "une seule ligne", mais je m'en tenir à ce que la base de code utilise normalement.

Lorsque vous utilisez ternary si la vérification logique rend la ligne trop longue ou compliquée, envisagez de créer une / des variables bien nommées pour contenir la valeur.

// NOTE "resourceId" not present in dev build, use test data
let isProduction = 'resourceId' in headers;
let phoneNumber = isProduction ? headers.resourceId : DEV_PHONE_NUMBER;

Je voudrais également dire que si la base de code s’étend sur 300 fonctions de ligne, elle nécessite une subdivision. Mais je conseillerais l'utilisation de traits légèrement plus larges.

nepeo
la source
5

L'exemple de code que vous avez donné, votre patron est correct. Une seule ligne claire est préférable dans ce cas.

En général, diviser la logique complexe en éléments plus petits est préférable pour la lisibilité, la maintenance du code et la possibilité que les sous-classes aient un comportement différent (même si ce n'est que légèrement).

N'ignorez pas les inconvénients: surcharge de la fonction, obscurcissement (la fonction ne fait pas ce que les commentaires et le nom de la fonction impliquent), la logique complexe des spaghettis, le potentiel de fonctions inactives (à une époque, ont été créées dans un but qui n'est plus appelé).

Phil M
la source
1
"fonction overhead": cela dépend du compilateur. "obscuration": OP n'a pas indiqué s'il s'agissait du seul ou du meilleur moyen de vérifier cette propriété; vous ne pouvez pas savoir avec certitude non plus. "logique complexe des spaghettis": où? "le potentiel de fonctions mortes": ce genre d'analyse de code mort est un fruit à portée de main, et les chaînes d'outils de développement qui en font défaut sont immatures.
Rhymoid
Les réponses étaient davantage axées sur les avantages, je voulais seulement souligner les inconvénients. Appeler une fonction telle que sum (a, b) sera toujours plus coûteux que "a + b" (à moins que la fonction ne soit insérée par le compilateur). Les autres inconvénients démontrent que la complexité excessive peut entraîner son propre ensemble de problèmes. Un code incorrect est un code incorrect, et le fait qu’il soit divisé en octets plus petits (ou conservé dans une boucle de 300 lignes) ne signifie pas qu’il est plus facile à avaler.
Phil M
2

Je peux penser à au moins deux arguments en faveur de fonctions longues:

  • Cela signifie que vous avez beaucoup de contexte autour de chaque ligne. Une façon de formaliser ceci: dessinez le graphe de flux de contrôle de votre code. À un sommet (~ = ligne) entre l'entrée de fonction et la sortie de fonction, vous connaissez tous les fronts entrants. Plus la fonction est longue, plus il y a de tels sommets.

  • Beaucoup de petites fonctions signifient qu'il existe un graphe d'appels plus grand et plus complexe. Choisissez une ligne aléatoire dans une fonction aléatoire et répondez à la question "dans quel contexte cette ligne est-elle exécutée?" Cela devient d'autant plus difficile que le graphe d'appel est volumineux et complexe, car il doit examiner plus de sommets dans ce graphe.

Il existe également des arguments contre les fonctions longues, comme la possibilité de tester l’unité. Servez-vous de votre expérience pour choisir entre l’une et l’autre.

Note: Je ne dis pas que votre patron a raison, seulement que son point de vue peut ne pas être complètement dépourvu de valeur.


Je pense que mon point de vue est que le bon paramètre d'optimisation n'est pas la longueur de la fonction. Je pense qu'il est plus utile de penser en termes de desiderata comme suit: toutes choses étant égales par ailleurs, il est préférable de pouvoir extraire du code une description détaillée de la logique métier et de la mise en œuvre. (Les détails de l'implémentation de bas niveau peuvent toujours être lus si vous pouvez trouver le bit de code approprié.)


Commentant la réponse de David Arno :

Écrire de petites fonctions est une tâche pénible, car il vous oblige à vous déplacer dans chaque petite fonction pour voir ce que le code fait.

Si la fonction est bien nommée, ce n'est pas le cas. isApplicationInProduction va de soi et il ne devrait pas être nécessaire d'examiner le code pour voir ce qu'il fait. En fait, le contraire est vrai: l’examen du code révèle moins quant à l’intention que le nom de la fonction (c’est pourquoi votre patron doit recourir à des commentaires).

Le nom met en évidence ce que la valeur de retour signifie , mais il ne dit rien sur les effets de l' exécution du code (= ce que le code fait ). Les noms (uniquement) transmettent des informations sur l' intention , le code transmet des informations sur le comportement (à partir desquelles des parties de l'intention peuvent parfois être déduites).

Parfois, vous voulez l'un, parfois l'autre, pour que cette observation ne crée pas une règle de décision unilatérale universellement valable.

Mettez tout dans une grande boucle principale même si la boucle principale compte plus de 300 lignes, la lecture est plus rapide

La numérisation est peut-être plus rapide, mais pour vraiment "lire" le code, vous devez être capable de l'exécuter efficacement dans votre tête. C'est facile avec de petites fonctions et c'est vraiment très dur avec des méthodes de plusieurs centaines de lignes.

Je suis d'accord que vous devez l'exécuter dans votre tête. Si vous avez 500 lignes de fonctionnalités dans une seule grande fonction par rapport à de nombreuses petites fonctions, je ne comprends pas pourquoi cela devient plus facile.

Supposons le cas extrême de 500 lignes de code en ligne droite ayant un effet très secondaire et si vous voulez savoir si l’effet A se produit avant ou après l’effet B. Dans le cas d’une grande fonction, utilisez Page Haut / Bas pour localiser deux lignes, puis comparez numéros de ligne. Dans le cas des nombreuses petites fonctions, vous devez vous rappeler où se produisent les effets dans l'arborescence des appels et, si vous oubliez, vous devez passer une quantité non négligeable de temps à redécouvrir la structure de cet arborescence.

Lorsque vous parcourez l'arborescence des appels des fonctions de support, vous devez également déterminer quand passer de la logique métier aux détails de la mise en œuvre. Je prétends sans preuve * que plus le graphe d’appel est simple, plus il est facile de faire cette distinction.

(*) Au moins, je suis honnête à ce sujet ;-)

Encore une fois, je pense que les deux approches ont des forces et des faiblesses.

N'écrivez que de petites fonctions si vous devez dupliquer le code

Je ne suis pas d'accord. Comme votre exemple de code le montre, de petites fonctions bien nommées améliorent la lisibilité du code et doivent être utilisées chaque fois que [par exemple] vous n'êtes pas intéressé par le "comment", mais uniquement par le "quoi" d'une fonctionnalité.

Que vous soyez intéressé par le "comment" ou le "quoi" dépend du but pour lequel vous lisez le code (par exemple, avoir une idée générale plutôt que de traquer un bogue). Le but pour lequel vous lisez le code n'est pas disponible lors de l'écriture du programme et vous lirez probablement le code à des fins différentes; différentes décisions seront optimisées à des fins différentes.

Cela dit, c’est le point de vue du patron avec lequel je suis probablement le plus en désaccord.

N'écrivez pas une fonction avec le nom du commentaire, mettez votre ligne de code complexe (3-4 lignes) avec un commentaire ci-dessus. Comme cela, vous pouvez modifier le code défaillant directement

Je ne peux vraiment pas comprendre le raisonnement derrière celui-ci, en supposant que ce soit vraiment sérieux. [...] Les commentaires ont un défaut fondamental: ils ne sont pas compilés / interprétés et ne peuvent donc pas être testés à l'unité. Le code est modifié et le commentaire est laissé seul et vous finissez par ne pas savoir qui est correct.

Les compilateurs ne comparent que les noms pour l’égalité, ils ne vous donnent jamais une MisleadingNameError. De plus, étant donné que plusieurs sites d’appel peuvent appeler une fonction donnée par son nom, il est parfois plus ardu et plus sujet aux erreurs de changer de nom. Les commentaires n'ont pas ce problème. Cependant, ceci est quelque peu spéculatif; pour vraiment régler ce problème, il faudrait probablement des données sur le point de savoir si les programmeurs sont plus susceptibles de mettre à jour des commentaires trompeurs par rapport à des noms trompeurs, et je n'ai pas cela.

Jonas Kölker
la source
-1

À mon avis, le code correct pour la fonctionnalité requise est le suivant:

phoneNumber = headers.resourceId || DEV_PHONE_NUMBER;

Ou si vous voulez le scinder en une fonction, probablement quelque chose comme:

phoneNumber = getPhoneNumber(headers);

function getPhoneNumber(headers) {
  return headers.resourceId || DEV_PHONE_NUMBER
}

Mais je pense que vous avez un problème plus fondamental avec le concept de "en production". Le problème avec votre fonction isApplicationInProductionest qu’il semble étrange que c’est le seul endroit du système où la "production" a son importance, et que vous pouvez toujours compter sur la présence ou non de l’en-tête resourceId pour vous en informer. Il devrait y avoir une isApplicationInProductionméthode générale ou une getEnvironmentméthode qui vérifie directement l'environnement. Le code devrait ressembler à:

function isApplicationInProduction() {
  process.env.NODE_ENV === 'production';
}

Ensuite, vous pouvez obtenir le numéro de téléphone avec:

phoneNumber = isApplicationInProduction() ? headers.resourceId : DEV_PHONE_NUMBER;
Rjmunro
la source
-2

Juste un commentaire sur deux des points de balle

  • Écrire de petites fonctions est une tâche pénible car cela vous oblige à vous déplacer dans chaque petite fonction pour voir ce que le code fait.
  • Mettez tout dans une grande boucle principale même si la boucle principale compte plus de 300 lignes, la lecture est plus rapide.

De nombreux éditeurs (par exemple, IntelliJ) vous permettent de passer directement à une fonction / classe simplement en maintenant la touche Ctrl enfoncée. De plus, vous n'avez souvent pas besoin de connaître les détails d'implémentation d'une fonction pour lire le code, ce qui accélère sa lecture.

Je vous recommande de le dire à votre patron. il aimera votre plaidoyer et le considérera comme un leadership. Sois juste poli.

noɥʇʎԀʎzɐɹƆ
la source