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

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?

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).

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.

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.

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.

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é).