¿Soy demasiado 'inteligente' para que los desarrolladores Jr. puedan leerlo? ¿Demasiada programación funcional en mi JS? [cerrado]

131

Soy un desarrollador principal Sr., que codifica en Babel ES6. Parte de nuestra aplicación hace una llamada a la API y, según el modelo de datos que recibimos de la llamada a la API, se deben completar ciertos formularios.

Esos formularios se almacenan en una lista con doble enlace (si el back-end dice que algunos de los datos no son válidos, podemos hacer que el usuario vuelva rápidamente a la única página en la que se equivocó y luego volver a ponerlo en el objetivo, simplemente modificando la 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áginas diferentes, pero esto le dará un ejemplo.

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

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 verificable, tomé todas esas declaraciones if y las hice funciones independientes, independientes, y luego las mapeo sobre ellas.

Ahora, verificable 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 yo el fondo está más organizado. El código en sí se divide en partes más pequeñas que se pueden probar de forma aislada. PERO estoy pensando: si tuviera que leer eso como un desarrollador junior, no acostumbrado a conceptos como el uso de los funtores de Identidad, el curry o las declaraciones ternarias, ¿sería capaz de entender qué hace esta última solución? ¿Es mejor hacer las cosas de la manera "incorrecta, más fácil" a veces?

    
pregunta Brian Boyko 06.06.2017 - 06:52

5 respuestas

320

En tu código, has realizado varios cambios:

  • desestructurar la asignación para acceder a los campos en pages es un buen cambio.
  • extraer las funciones parseFoo() etc. es un posible cambio.
  • introducir un funtor es ... muy confuso.

Una de las partes más confusas aquí es cómo se mezcla la programación funcional y la imperativa. Con su functor no está realmente 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, sigue estando explícitamente en su código, pero ahora tenemos que pensar a la vuelta de la esquina. Por ejemplo, es algo 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 movido a las funciones de análisis individuales. Si esto no es apropiado, todavía puede introducir condicionales. La mutabilidad de la lista pages ahora se agrupa en una sola función, en lugar de distribuirla en múltiples llamadas. Evitar las mutaciones no locales es una parte mucho más importante de la programación funcional que las abstracciones con nombres divertidos como Monad .

Así que sí, tu 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 descarada. Los mejores diseños no son elegantes, pero sí obvios para cualquiera que los vea. Y hay buenas abstracciones para simplificar la programación, no para agregar capas adicionales que tengo que desentrañar en mi mente primero (aquí, descubrir que el functor es equivalente a una variable, y puede ser eliminado de manera efectiva).

Por favor: en caso de duda, mantenga su código simple y estúpido (principio KISS).

    
respondido por el amon 06.06.2017 - 08:43
223

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 beneficio pretendido, que puede probar los trozos individualmente, podría lograrse de una manera más simple simplemente rompiendo en funciones individuales. Y en el segundo ejemplo, se juntan las iteraciones separadas, por lo que en realidad se obtiene un aislamiento menos .

Cualquiera que sea su opinión sobre el estilo funcional en general, este es claramente un ejemplo en el que hace que el código sea más complejo.

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

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

    
respondido por el JacquesB 06.06.2017 - 08:45
20

Esta respuesta mía llega un poco tarde, pero aún así 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 El código de ese junior es incorrecto. La programación funcional (o cualquier otra técnica) debe usarse cuando sea realmente necesaria. Si intenta encontrar la más mínima posibilidad de incluir las últimas técnicas de programación en cada problema, siempre terminará con una solución de ingeniería excesiva.

Da un paso atrás e intenta verbalizar el problema que intentas resolver por un segundo. En esencia, solo desea que una función addPages transforme diferentes partes de apiData en un conjunto de pares clave-valor, luego agregue todos ellos en PagesList .

Y si eso es todo, ¿por qué molestarse en usar identity function con ternary operator , o usar functor para el análisis de entrada? Además, ¿por qué crees que es un enfoque adecuado para aplicar functional programming que causa efectos secundarios ( mutando la lista)? Por qué todas esas cosas, cuando todo lo que necesitas es simplemente 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 usa functional programming , pero con moderación. También tenga en cuenta que, dado que las 3 funciones de transformación no causan efectos secundarios, son fáciles de probar. El código en addPages también es trivial y sencillo, que los principiantes o expertos pueden entender con solo una simple mirada.

Ahora, compara este código con el resultado anterior, ¿ves la diferencia? Sin lugar a dudas, las sintaxis de functional programming y ES6 son poderosas, pero si corta el problema de manera incorrecta con estas técnicas, terminará con un código aún más desordenado.

Si no te apresuras en el problema y aplicas las técnicas correctas en los lugares correctos, puedes tener el código que es funcional en la naturaleza, mientras que todos los miembros del equipo pueden organizarlo y mantenerlo. Estas características no son mutuamente excluyentes.

    
respondido por el b0nyb0y 09.06.2017 - 21:30
5

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

La diferencia real entre los dos fragmentos es comprensible. Puedo leer el primer fragmento bastante rápido y entender lo que está pasando. 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, que es una valiosa calidad de código. Encuentro muy poco valor en el segundo fragmento.

    
respondido por el Dawood ibn Kareem 10.06.2017 - 09:45
3

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. Está en un estilo familiar para todos .

La legibilidad se basa en gran medida en la familiaridad, no en un conteo matemático de tokens . OMI, en esta etapa en el tiempo, tiene demasiado ES6 en su 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.

Probabilidad

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 separadas especiales.

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 realizar 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 customBazParser se prueba y no devuelve demasiados resultados "especiales". Entonces, una vez más, 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 declaraciones if() , entonces sí, divida las pruebas. O, si PagesList.add() es sensible, sí, divida las pruebas.

Estructura: vale la pena dividirse 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 los SRP). Pero, si hubiera unas pocas secciones if (apiData.pages.blah) más, entonces SRP levanta la cabeza y sería interesante dividirla. Especialmente si se aplica DRY y las funciones podrían usarse en otros lugares del código.

Mi única sugerencia

YMMV. Para guardar una línea de código y algo de lógica, podría combinar if y let en una línea: e.g.

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 Cadena, pero también lo hará el código original. Y para mí es más claro (y un lenguaje demasiado usado).

    
respondido por el user949300 10.06.2017 - 19:31