¿Soy demasiado 'inteligente' para ser legible por los desarrolladores de Jr.? ¿Demasiada programación funcional en mi JS? [cerrado]

Soy un desarrollador senior de front-end, codificando en Babel ES6. Parte de nuestra aplicación realiza una llamada a la API y, según el modelo de datos que recibimos de la llamada a la API, es necesario completar ciertos formularios.

Esos formularios se almacenan en una lista doblemente vinculada (si el back-end dice que algunos de los datos no son válidos, podemos hacer que el usuario regrese rápidamente a la página en la que se equivocaron y luego volver al objetivo, simplemente modificando el lista.)

De todos modos, hay un montón de funciones que se utilizan para agregar páginas, y me pregunto si soy demasiado inteligente. Esta es solo una descripción básica: el algoritmo real es mucho más complejo, con toneladas de páginas y tipos de página diferentes, pero esto le dará un ejemplo.

Así es como, creo, un programador novato lo manejaría.

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;
}

Ahora, para ser más comprobable, tomé todas esas declaraciones if y las hice separadas, funciones independientes, y luego las mapeo.

Ahora, comprobable es una cosa, pero también es legible y me pregunto si estoy haciendo las cosas menos legibles aquí.

// 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

}

Aquí está mi preocupación. Para mí, el fondo está más organizado. El código en sí se divide en fragmentos más pequeños que se pueden probar de forma aislada. PERO estoy pensando: si tuviera que leer eso como desarrollador junior, sin estar acostumbrado a conceptos como el uso de functores de identidad, currículum o declaraciones ternarias, ¿podría incluso entender lo que está haciendo la última solución? ¿Es mejor hacer las cosas de la manera "incorrecta y más fácil" a veces?

En su código, ha realizado múltiples cambios:

• La asignación de desestructuración para acceder a los campos en el pageses un buen cambio.
• extraer las parseFoo()funciones, etc. es un cambio posiblemente bueno.
• presentar un functor es ... muy confuso.

Una de las partes más confusas aquí es cómo está mezclando la programación funcional e imperativa. Con su functor realmente no está transformando datos, lo está utilizando para pasar una lista mutable a través de varias funciones. Eso no parece una abstracción muy útil, ya tenemos variables para eso. Lo que posiblemente debería haberse abstraído, solo analizando ese elemento si existe, todavía está allí en su código explícitamente, pero ahora tenemos que pensar a la vuelta de la esquina. Por ejemplo, es algo no obvio que parseFoo(foo)devolverá una función. JavaScript no tiene un sistema de tipo estático para notificarle si esto es legal, por lo que dicho código es realmente propenso a errores sin un nombre mejor ( makeFooParser(foo)?). No veo ningún beneficio en esta ofuscación.

Lo que esperaría ver en su lugar:

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

Pero eso tampoco es ideal, porque no está claro en el sitio de la llamada que los elementos se agregarán a la lista de páginas. Si, en cambio, las funciones de análisis son puras y devuelven una lista (posiblemente vacía) que podemos agregar explícitamente a las páginas, podría ser mejor:

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

Beneficio adicional: la lógica sobre qué hacer cuando el elemento está vacío ahora se ha trasladado a las funciones de análisis individuales. Si esto no es apropiado, aún puede introducir condicionales. La mutabilidad de la pageslista ahora se agrupa en una sola función, en lugar de distribuirla en varias llamadas. Evitar mutaciones no locales es una parte mucho mayor de la programación funcional que las abstracciones con nombres divertidos como Monad.

Entonces sí, su código era demasiado inteligente. Aplique su inteligencia no para escribir código inteligente, sino para encontrar formas inteligentes de evitar la necesidad de una inteligencia evidente. Los mejores diseños no parecen elegantes, pero son obvios para cualquiera que los vea. Y hay buenas abstracciones para simplificar la programación, no para agregar capas adicionales que primero tengo que desenredar en mi mente (aquí, descubriendo que el functor es equivalente a una variable y puede ser eliminado de manera efectiva).

Por favor: si tiene dudas, mantenga su código simple y estúpido (principio KISS).

Si tiene dudas, ¡probablemente sea demasiado inteligente! El segundo ejemplo introduce complejidad accidental con expresiones como foo ? parseFoo(foo) : x => x, y en general el código es más complejo, lo que significa que es más difícil de seguir.

El supuesto beneficio, de que puede probar los trozos individualmente, podría lograrse de una manera más simple simplemente dividiendo las funciones individuales. Y en el segundo ejemplo, combina las iteraciones separadas, por lo que en realidad obtienes menos aislamiento.

Sean cuales sean sus sentimientos sobre el estilo funcional en general, este es claramente un ejemplo en el que hace que el código sea más complejo.

Encuentro un poco de señal de advertencia en el sentido de que asocias un código simple y directo con "desarrolladores novatos". Esta es una mentalidad peligrosa. En mi experiencia, es todo lo contrario: los desarrolladores novatos son propensos a un código demasiado complejo e inteligente, porque requiere más experiencia para poder ver la solución más simple y clara.

El consejo contra el "código inteligente" no se trata realmente de si el código usa o no conceptos avanzados que un principiante podría no entender. Más bien se trata de escribir código que sea más complejo o complicado de lo necesario . Esto hace que el código sea más difícil de seguir para todos , principiantes y expertos por igual, y probablemente también para usted algunos meses más adelante.

Esta respuesta mía llega un poco tarde, pero todavía quiero intervenir. El hecho de que esté utilizando las últimas técnicas de ES6 o el paradigma de programación más popular no significa necesariamente que su código sea más correcto o que el código de junior Está Mal. La programación funcional (o cualquier otra técnica) debe usarse cuando realmente se necesita. Si intenta encontrar la mínima posibilidad de incluir las últimas técnicas de programación en cada problema, siempre terminará con una solución sobredimensionada.

Da un paso atrás e intenta verbalizar el problema que estás tratando de resolver por un segundo. En esencia, solo desea una función addPagespara transformar diferentes partes de apiDataen un conjunto de pares clave-valor, luego agregarlos a todos PagesList.

Y si eso es todo, ¿por qué molestarse en usar identity functioncon ternary operator, o usar functorpara el análisis de entrada? Además, ¿por qué crees que es un enfoque adecuado para aplicar functional programmingque causa efectos secundarios (al mutar la lista)? Por qué todas esas cosas, cuando todo lo que necesitas es solo esto:

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 ejecutable aquí )

Como puede ver, este enfoque todavía se usa functional programming, pero con moderación. También tenga en cuenta que dado que las 3 funciones de transformación no causan ningún efecto secundario, son extremadamente fáciles de probar. El código addPagestambién es trivial y sin pretensiones que los principiantes o expertos pueden entender con solo una simple mirada.

Ahora, compare este código con lo que se le ocurrió anteriormente, ¿ve la diferencia? Sin duda, functional programmingy las sintaxis de ES6 son poderosas, pero si corta el problema de la manera incorrecta con estas técnicas, terminará con un código aún más desordenado.

Si no se apresura en el problema y aplica las técnicas correctas en los lugares correctos, puede tener el código que es funcional en la naturaleza, mientras que todavía es muy organizado y mantenible por todos los miembros del equipo. Estas características no son mutuamente excluyentes.

El segundo fragmento no es más comprobable que el primero. Sería razonablemente sencillo configurar todas las pruebas necesarias para cualquiera de los dos fragmentos.

La verdadera diferencia entre los dos fragmentos es la comprensibilidad. Puedo leer el primer fragmento bastante rápido y entender lo que está sucediendo. El segundo fragmento, no tanto. Es mucho menos intuitivo, así como sustancialmente más largo.

Eso hace que el primer fragmento sea más fácil de mantener, lo cual es una valiosa calidad de código. Encuentro muy poco valor en el segundo fragmento.

TD; DR

1. ¿Puede explicar su código al Desarrollador Junior en 10 minutos o menos?
2. Dentro de dos meses, ¿puedes entender tu código?

Análisis detallado

Claridad y legibilidad

El código original es impresionantemente claro y fácil de entender para cualquier nivel de programador. Tiene un estilo familiar para todos .

La legibilidad se basa en gran medida en la familiaridad, no en un recuento matemático de tokens . En mi opinión, en este momento, tienes demasiado ES6 en tu reescritura. Tal vez en un par de años cambie esta parte de mi respuesta. :-) Por cierto, también me gusta la respuesta @ b0nyb0y como un compromiso razonable y claro.

Testabilidad

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

Suponiendo que PagesList.add () tiene pruebas, lo cual debería ser, este es un código completamente sencillo y no hay una razón obvia para que esta sección necesite pruebas especiales por separado.

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

Nuevamente, no veo la necesidad inmediata de ninguna prueba especial por separado de esta sección. A menos que PagesList.add () tenga problemas inusuales con nulos o duplicados u otras entradas.

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

Este código también es muy sencillo. Suponiendo que customBazParserse prueba y no devuelve demasiados resultados "especiales". Entonces, de nuevo, a menos que haya situaciones difíciles con `PagesList.add (), (que podría haber ya que no estoy familiarizado con su dominio) No veo por qué esta sección necesita pruebas especiales.

En general, probar toda la función debería funcionar bien.

Descargo de responsabilidad : si hay razones especiales para probar las 8 posibilidades de las tres if()declaraciones, entonces sí, divida las pruebas. O, si PagesList.add()es sensible, sí, divida las pruebas.

Estructura: ¿Vale la pena dividirlo en tres partes (como la Galia)

Aquí tienes el mejor argumento. Personalmente, no creo que el código original sea "demasiado largo" (no soy un fanático de SRP). Pero, si hubiera algunas if (apiData.pages.blah)secciones más , entonces SRP muestra su fea cabeza y valdría la pena dividirla. Especialmente si se aplica DRY y las funciones podrían usarse en otros lugares del código.

Mi sugerencia

YMMV. Para guardar una línea de código y algo de lógica, podría combinar el if y dejarlo en una línea: por ej.

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

Esto fallará si apiData.pages.arrayOfBars es un Número o una Cadena, pero también lo hará el código original. Y para mí es más claro (y un idioma usado en exceso).