Suis-je trop «intelligent» pour être lisible par les développeurs Jr.? Trop de programmation fonctionnelle dans mon JS? [fermé]

133

Je suis un développeur Sr front-end, codant dans Babel ES6. Une partie de notre application effectue un appel API et, en fonction du modèle de données que nous avons renvoyé, certains formulaires doivent être remplis.

Ces formulaires sont stockés dans une liste à double lien (si le back-end indique que certaines données sont invalides, nous pouvons rapidement ramener l'utilisateur à la page qu'il a foirée, puis le ramener sur la cible, simplement en modifiant le liste.)

Quoi qu'il en soit, de nombreuses fonctions sont utilisées pour ajouter des pages, et je me demande si je ne suis pas trop intelligent. Ceci est juste une vue d'ensemble - l'algorithme lui-même est beaucoup plus complexe, avec des tonnes de pages et de types de page différents, mais ceci vous donnera un exemple.

C'est comment, je pense, un programmeur novice le gérerait.

export const addPages = (apiData) => {
   let pagesList = new PagesList(); 

   if(apiData.pages.foo){
     pagesList.add('foo', apiData.pages.foo){
   }

   if (apiData.pages.arrayOfBars){
      let bars = apiData.pages.arrayOfBars;
      bars.forEach((bar) => {
         pagesList.add(bar.name, bar.data);
      })
   }

   if (apiData.pages.customBazes) {
      let bazes = apiData.pages.customBazes;
      bazes.forEach((baz) => {
         pagesList.add(customBazParser(baz)); 
      })
   } 

   return pagesList;
}

Maintenant, afin d’être plus testable, j’ai pris toutes ces déclarations if et les ai séparées, en fonctions autonomes, puis mappées sur elles.

Maintenant, le testable est une chose, mais il en est de même pour la lecture et je me demande si je le fais moins ici.

// file: '../util/functor.js'

export const Identity = (x) => ({
  value: x,
  map: (f) => Identity(f(x)),
})

// file 'addPages.js' 

import { Identity } from '../util/functor'; 

export const parseFoo = (data) => (list) => {
   list.add('foo', data); 
}

export const parseBar = (data) => (list) => {
   data.forEach((bar) => {
     list.add(bar.name, bar.data)
   }); 
   return list; 
} 

export const parseBaz = (data) => (list) => {
   data.forEach((baz) => {
      list.add(customBazParser(baz)); 
   })
   return list;
}


export const addPages = (apiData) => {
   let pagesList = new PagesList(); 
   let { foo, arrayOfBars: bars, customBazes: bazes } = apiData.pages; 

   let pages = Identity(pagesList); 

   return pages.map(foo ? parseFoo(foo) : x => x)
               .map(bars ? parseBar(bars) : x => x)
               .map(bazes ? parseBaz(bazes) : x => x)
               .value

}

Voici ma préoccupation. Pour moi, le fond est plus organisé. Le code lui-même est divisé en petits morceaux qui peuvent être testés séparément. MAIS je pense: si je devais lire cela en tant que développeur junior, peu habitué aux concepts tels que l’utilisation de foncteurs d’identité, le currying ou les déclarations ternaires, serais-je même en mesure de comprendre ce que cette dernière solution fait? Est-il préférable de faire les choses de la manière "fausse, facile" parfois?

Brian Boyko
la source
13
en tant que personne qui a seulement 10 ans d' Babel ES6
autoapprentissage
26
OMG - vous travaillez dans l'industrie depuis 1999 et codez depuis 1983, et vous êtes le type de développeur le plus dangereux qui soit. Ce que vous pensez être "intelligent" est vraiment appelé "cher", "difficile à maintenir" et "source d'insectes" et n'a pas sa place dans un environnement professionnel. Le premier exemple est simple, facile à comprendre et fonctionne, tandis que le second est complexe, difficile à comprendre et ne peut pas être corrigé. S'il vous plaît, arrêtez de faire ce genre de chose. Ce n'est pas meilleur, sauf peut-être dans un sens académique qui ne s'applique pas au monde réel.
user1068
15
Je voudrais juste citer Brian Kerninghan: "Tout le monde sait que déboguer est deux fois plus difficile que d’écrire un programme. Donc, si vous êtes aussi intelligent que vous le pouvez lorsque vous l’écrivez, comment allez-vous le déboguer? " - en.wikiquote.org/wiki/Brian_Kernighan / "Les éléments du style de programmation", 2e édition, chapitre 2.
MarkusSchaber Le
7
@Logister Coolness n'est pas plus un objectif premier que la simplicité. L'objection ici est à la complexité gratuite , qui est l'ennemi de la correction (une préoccupation primordiale) parce qu'elle rend le code plus difficile à raisonner et plus susceptible de contenir des cas inattendus. Étant donné le scepticisme que j'ai déjà exprimé à l'égard des affirmations selon lesquelles il est en fait plus facile à tester, je n'ai vu aucun argument convaincant pour ce style. Par analogie avec la règle de la sécurité minimale des privilèges en matière de sécurité, il pourrait peut-être exister une règle empirique qui dit de ne pas utiliser de puissantes fonctionnalités de langage pour faire des choses simples.
Sdenham
6
Votre code ressemble au code junior. Je m'attendrais à ce qu'une personne âgée écrive le premier exemple.
sed

Réponses:

322

Dans votre code, vous avez effectué plusieurs modifications:

  • L’attribution de déstructuration pour accéder aux champs de la pagesest un bon changement.
  • extraire les parseFoo()fonctions, etc. est un bon changement.
  • Présenter un foncteur est… très déroutant.

Une des parties les plus déroutantes est la façon dont vous mélangez la programmation fonctionnelle et impérative. Avec votre foncteur, vous ne transformez pas vraiment les données, vous les utilisez pour passer une liste modifiable à travers diverses fonctions. Cela ne semble pas être une abstraction très utile, nous avons déjà des variables pour cela. La chose qui aurait éventuellement dû être abstraite - uniquement l'analyse de cet élément s'il existe - est toujours explicitement dans votre code, mais nous devons maintenant réfléchir au coin de la rue. Par exemple, il est quelque peu non évident de parseFoo(foo)retourner une fonction. JavaScript n'a pas de système de type statique pour vous avertir si cela est légal. Un tel code est donc sujet aux erreurs sans un meilleur nom ( makeFooParser(foo)?). Je ne vois aucun avantage dans cette obfuscation.

Ce que je m'attendrais à voir à la place:

if (foo) parseFoo(pages, foo);
if (bars) parseBar(pages, bars);
if (bazes) parseBaz(pages, bazes);
return pages;

Mais ce n'est pas idéal non plus, car il n'est pas clair à partir du site d'appels que les éléments seront ajoutés à la liste des pages. Si au contraire, les fonctions d'analyse sont pures et renvoient une liste (éventuellement vide) que nous pouvons explicitement ajouter aux pages, cela pourrait être mieux:

pages.addAll(parseFoo(foo));
pages.addAll(parseBar(bars));
pages.addAll(parseBaz(bazes));
return pages;

Avantage supplémentaire: la logique de ce qu'il faut faire lorsque l'élément est vide a maintenant été déplacée dans les fonctions d'analyse syntaxique individuelles. Si cela ne convient pas, vous pouvez toujours introduire des conditions. La mutabilité de la pagesliste est maintenant regroupée dans une seule fonction, au lieu de la répartir sur plusieurs appels. Éviter les mutations non locales est une partie beaucoup plus importante de la programmation fonctionnelle que les abstractions avec des noms amusants comme Monad.

Alors oui, votre code était trop intelligent. Appliquez votre intelligence non pas pour écrire un code intelligent, mais pour trouver des moyens astucieux d'éviter le besoin d'une intelligence flagrante. Les meilleures conceptions ne semblent pas sophistiquées, mais paraissent évidentes à quiconque les voit. Et les bonnes abstractions sont là pour simplifier la programmation, pas pour ajouter des couches supplémentaires que je dois d'abord démêler dans mon esprit (ici, déterminer que le foncteur est équivalent à une variable et peut effectivement être éliminé).

S'il vous plaît: en cas de doute, gardez votre code simple et stupide (principe KISS).

Amon
la source
2
Du point de vue de la symétrie, en quoi let pages = Identity(pagesList)diffère- parseFoo(foo)t-il? Étant donné que, je l' aurais probablement ... {Identity(pagesList), parseFoo(foo), parseBar(bar)}.flatMap(x -> x).
ArTs
8
Merci d’avoir expliqué qu’avoir trois expressions lambda imbriquées pour la collecte d’une liste mappée (à mon œil inexpérimenté) est peut-être un peu trop malin.
Thorbjørn Ravn Andersen
2
Les commentaires ne sont pas pour une discussion prolongée; cette conversation a été déplacée pour discuter .
Yannis
Peut-être qu'un style fluide fonctionnerait bien dans votre deuxième exemple?
user1068
225

Si vous avez des doutes, c'est probablement trop malin! Le deuxième exemple introduit une complexité accidentelle avec des expressions telles que foo ? parseFoo(foo) : x => x, et dans l’ensemble, le code est plus complexe, ce qui signifie qu’il est plus difficile à suivre.

L'avantage supposé, à savoir que vous pouvez tester les morceaux individuellement, pourrait être obtenu d'une manière plus simple en entrant simplement dans des fonctions individuelles. Et dans le deuxième exemple, vous associez les itérations par ailleurs séparées, ce qui vous donne moins d' isolement.

Quels que soient vos sentiments sur le style fonctionnel en général, il s'agit clairement d'un exemple dans lequel le code est plus complexe.

Je trouve un signe d’avertissement en ce sens que vous associez du code simple et direct à des "développeurs novices". C'est une mentalité dangereuse. D'après mon expérience, c'est l'inverse qui se produit: les développeurs novices sont enclins à utiliser un code trop complexe et intelligent, car il leur faut plus d'expérience pour pouvoir visualiser la solution la plus simple et la plus claire.

Le conseil contre le "code intelligent" n'est pas vraiment de savoir si le code utilise des concepts avancés qu'un novice pourrait ne pas comprendre. Il s’agit plutôt d’écrire un code plus complexe ou compliqué que nécessaire . Cela rend le code plus difficile à suivre pour tout le monde , novices et experts, et probablement aussi pour vous-même quelques mois plus tard.

JacquesB
la source
156
"Les développeurs novices sont enclins à utiliser un code trop complexe et intelligent, car il leur faut plus d'expérience pour pouvoir voir la solution la plus simple et la plus claire" ne peut pas être plus d'accord avec vous. Excellente réponse!
Bonifacio
23
Un code trop complexe est également assez passif-agressif. Vous produisez délibérément du code que peu de gens peuvent lire ou déboguer facilement ... ce qui signifie sécurité d'emploi pour vous et enfer pour tous les autres en votre absence. Vous pouvez également rédiger votre documentation technique entièrement en latin.
Ivan
14
Je ne pense pas qu'un code intelligent soit toujours une démonstration. Parfois, cela semble naturel et ne semble ridicule que lors d'une seconde inspection.
5
J'ai supprimé le phrasé sur "montrer" parce que cela sonnait plus de jugement que prévu.
JacquesB
11
@BaileyS - Je pense que cela souligne l'importance de la révision du code; ce qui semble naturel et simple pour le codeur, surtout quand il est développé de cette manière, peut sembler compliqué pour un relecteur. Le code ne passe ensuite pas la révision avant d'avoir été refactorisé / réécrit pour supprimer la convolution.
Myles
21

Cette réponse m'est un peu tardive, mais je veux tout de même y répondre. Ce n'est pas parce que vous utilisez les dernières techniques ES6 ou le paradigme de programmation le plus populaire que votre code est plus correct, ou que le code de ce junior est faux. La programmation fonctionnelle (ou toute autre technique) doit être utilisée quand elle est réellement nécessaire. Si vous essayez de trouver la moindre chance d'intégrer les dernières techniques de programmation à chaque problème, vous obtiendrez toujours une solution sur-conçue.

Faites un pas en arrière et essayez de verbaliser le problème que vous essayez de résoudre une seconde. Essentiellement, vous voulez simplement qu'une fonction addPagestransforme différentes parties apiDataen un ensemble de paires clé-valeur, puis les ajoute toutes PagesList.

Et si c'est tout ce qu'il ya à faire, pourquoi ne pas utiliser identity functionavec ternary operator, ou utiliser functorpour l'analyse syntaxique des entrées? En outre, pourquoi pensez-vous que c'est une approche appropriée à appliquer functional programmingqui provoque des effets secondaires (en modifiant la liste)? Pourquoi toutes ces choses, alors que tout ce dont vous avez besoin est juste ceci:

const processFooPages = (foo) => foo ? [['foo', foo]] : [];
const processBarPages = (bar) => bar ? bar.map(page => [page.name, page.data]) : [];
const processBazPages = (baz) => baz ? baz.map(page => [page.id, page.content]) : [];

const addPages = (apiData) => {
  const list = new PagesList();
  const pages = [].concat(
    processFooPages(apiData.pages.foo),
    processBarPages(apiData.pages.arrayOfBars),
    processBazPages(apiData.pages.customBazes)
  );
  pages.forEach(([pageName, pageContent]) => list.addPage(pageName, pageContent));

  return list;
}

(un jsfiddle exécutable ici )

Comme vous pouvez le constater, cette approche utilise toujours functional programming, mais avec modération. Notez également que, puisque les 3 fonctions de transformation ne provoquent aucun effet secondaire, elles sont extrêmement faciles à tester. Le code addPagesest également trivial et sans prétention que les novices ou les experts peuvent comprendre en un simple coup d’œil.

Maintenant, comparez ce code avec ce que vous avez trouvé ci-dessus, voyez-vous la différence? Il ne fait aucun doute functional programmingque les syntaxes ES6 sont puissantes, mais si vous divisez le problème de façon erronée avec ces techniques, vous obtiendrez un code encore plus compliqué.

Si vous ne vous précipitez pas dans le problème et que vous appliquez les bonnes techniques aux bons endroits, vous pouvez obtenir un code fonctionnel par nature, tout en restant très organisé et maintenable par tous les membres de l'équipe. Ces caractéristiques ne sont pas mutuellement exclusives.

b0nyb0y
la source
2
+1 pour signaler cette attitude répandue (ne s’applique pas nécessairement au PO): "Le fait que vous utilisiez les dernières techniques ES6 ou le paradigme de programmation le plus populaire ne signifie pas nécessairement que votre code est plus correct, ou le code de ce junior est faux. "
Giorgio
+1 Juste une petite remarque pédante, vous pouvez utiliser l'opérateur spread (...) au lieu de _.concat pour supprimer cette dépendance.
YoTengoUnLCD
1
@YoTengoUnLCD Ah, bonne prise. Vous savez maintenant que mon équipe et moi-même sommes toujours en train de désapprendre certaines de nos lodashutilisations. Ce code peut utiliser spread operator, ou même [].concat()si l'on veut garder la forme du code intact.
b0nyb0y
Désolé, mais cette liste de code est encore beaucoup moins évidente pour moi que le "code junior" original dans le post d'OP. Fondamentalement: N'utilisez jamais l'opérateur ternaire si vous pouvez l'éviter. C'est trop tendu. Dans un langage fonctionnel "réel", les instructions if seraient des expressions et non des instructions, et seraient donc plus lisibles.
Olle Härstedt
@ OlleHärstedt Umm, c'est une affirmation intéressante que vous avez faite. En réalité, le paradigme de la programmation fonctionnelle, ou n’importe quel paradigme, n’est jamais lié à un langage fonctionnel "réel" particulier, et encore moins à sa syntaxe. Ainsi, dicter quelles constructions conditionnelles devraient ou ne devraient jamais être utilisées n’a aucun sens. A ternary operatorest aussi valide qu’une ifdéclaration régulière , que cela vous plaise ou non. Le débat sur la lisibilité entre if-elseet le ?:camp est sans fin, je ne vais donc pas y entrer. Tout ce que je dirai, c'est qu'avec des yeux exercés, des lignes comme celles-ci ne sont guère "trop ​​tendues".
b0nyb0y
5

Le deuxième extrait n'est pas plus testable que le premier. Il serait assez simple de configurer tous les tests nécessaires pour l’un ou l’autre des deux extraits.

La vraie différence entre les deux extraits est la compréhensibilité. Je peux lire le premier extrait assez rapidement et comprendre ce qui se passe. Le deuxième extrait, pas tellement. C'est beaucoup moins intuitif, mais aussi beaucoup plus long.

Cela facilite la maintenance du premier extrait, ce qui constitue une qualité de code précieuse. Je trouve très peu de valeur dans le deuxième extrait.

Dawood ibn Kareem
la source
3

TD; DR

  1. Pouvez-vous expliquer votre code au développeur junior en 10 minutes ou moins?
  2. Dans deux mois, pouvez-vous comprendre votre code?

Analyse détaillée

Clarté et lisibilité

Le code original est incroyablement clair et facile à comprendre pour tous les niveaux de programmeur. C'est un style familier à tout le monde .

La lisibilité repose en grande partie sur la familiarité et non sur un comptage mathématique de jetons . OMI, à ce stade, vous avez trop d’ES6 dans votre réécriture. Peut-être que dans quelques années, je changerai cette partie de ma réponse. :-) BTW, j'aime aussi la réponse @ b0nyb0y comme un compromis raisonnable et clair.

Testabilité

if(apiData.pages.foo){
   pagesList.add('foo', apiData.pages.foo){
}

En supposant que PagesList.add () ait des tests, ce qui devrait être le cas, il s’agit d’un code tout à fait simple et il n’ya aucune raison évidente pour que cette section ait besoin de tests séparés spéciaux.

if (apiData.pages.arrayOfBars){
      let bars = apiData.pages.arrayOfBars;
      bars.forEach((bar) => {
         pagesList.add(bar.name, bar.data);
      })
   }

Encore une fois, je ne vois pas le besoin immédiat de tester séparément cette section. Sauf si PagesList.add () présente des problèmes inhabituels avec des valeurs NULL, des doublons ou d'autres entrées.

if (apiData.pages.customBazes) {
      let bazes = apiData.pages.customBazes;
      bazes.forEach((baz) => {
         pagesList.add(customBazParser(baz)); 
      })
   } 

Ce code est également très simple. En supposant que cela customBazParsersoit testé et ne renvoie pas trop de résultats "spéciaux". Encore une fois, à moins que `PagesList.add (), (car il se peut que je ne connaisse pas votre domaine), se trouve dans des situations difficiles, je ne vois pas pourquoi cette section nécessite des tests spéciaux.

En général, tester l'ensemble de la fonction devrait fonctionner correctement.

Clause de non - responsabilité : S'il existe des raisons particulières de tester les 8 possibilités des trois if()instructions, alors, effectivement, divisez les tests. Ou, si PagesList.add()est sensible, oui, divisez les tests.

Structure: faut-il diviser en trois parties (comme la Gaule)

Ici vous avez le meilleur argument. Personnellement, je ne pense pas que le code original soit "trop ​​long" (je ne suis pas un fanatique de SRP). Mais, s’il y avait quelques if (apiData.pages.blah)sections de plus , alors SRP l’avait mal vu et mériterait d’être divisé. Surtout si DRY s'applique et que les fonctions peuvent être utilisées à d’autres endroits du code.

Ma seule suggestion

YMMV. Pour enregistrer une ligne de code et une logique, je pourrais combiner le if et le laisser en une seule ligne: par exemple

let bars = apiData.pages.arrayOfBars || [];
bars.forEach((bar) => {
   pagesList.add(bar.name, bar.data);
})

Cela échouera si apiData.pages.arrayOfBars est un nombre ou une chaîne, mais le code d'origine le sera également. Et pour moi, c'est plus clair (et un idiome surutilisé).

utilisateur949300
la source