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

133

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?

Brian Boyko
fuente
13
como alguien que sólo tiene 10 años de auto-enseñanzas en JS, me consideraría una Jr. y me perdió enBabel ES6
ROZZA
26
OMG: ha estado activo en la industria desde 1999 y ha codificado desde 1983, y usted es el desarrollador más dañino que existe. Lo que usted piensa que es "inteligente" se llama realmente "costoso" y "difícil de mantener" y "una fuente de errores" y no tiene cabida en un entorno empresarial. El primer ejemplo es simple, fácil de entender y funciona, mientras que el segundo ejemplo es complejo, difícil de entender y no es probablemente correcto. Por favor, deja de hacer este tipo de cosas. NO es mejor, excepto quizás en algún sentido académico que no se aplica al mundo real.
user1068
15
Solo quiero citar a Brian Kerninghan aquí: "Todo el mundo sabe que la depuración es el doble de difícil que escribir un programa en primer lugar. Entonces, si es tan inteligente como puede ser cuando lo escribe, ¿cómo lo depurará alguna vez? " - en.wikiquote.org/wiki/Brian_Kernighan / "Los elementos del estilo de programación", segunda edición, capítulo 2.
MarkusSchaber
77
@Logister Coolness no es más un objetivo principal que la simplicidad. La objeción aquí es la complejidad gratuita , que es el enemigo de la corrección (seguramente una preocupación principal) porque hace que el código sea más difícil de razonar y es más probable que contenga casos de esquina inesperados. Dado mi escepticismo anterior de las afirmaciones de que en realidad es más fácil de probar, no he visto ningún argumento convincente para este estilo. En analogía con la regla de seguridad de wrt de privilegios mínimos, tal vez podría haber una regla general que diga que uno debe ser cauteloso al usar funciones de lenguaje potentes para hacer cosas simples.
sdenham
66
Su código se parece al código junior. Esperaría que un senior escriba el primer ejemplo.
sed

Respuestas:

322

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

amon
fuente
2
Desde un punto de vista de simetría, ¿qué let pages = Identity(pagesList)tiene diferente parseFoo(foo)? Teniendo en cuenta que, probablemente tendría ... {Identity(pagesList), parseFoo(foo), parseBar(bar)}.flatMap(x -> x).
ArTs
8
Gracias por explicar que tener tres expresiones lambda anidadas para recopilar una lista asignada (para mi ojo inexperto) podría ser un poco demasiado inteligente.
Thorbjørn Ravn Andersen
2
Los comentarios no son para discusión extendida; Esta conversación se ha movido al chat .
yannis
¿Quizás un estilo fluido funcionaría bien en tu segundo ejemplo?
user1068
225

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.

JacquesB
fuente
156
"Los desarrolladores novatos son propensos a un código excesivamente complejo e inteligente, porque requiere más experiencia para poder ver la solución más simple y clara" no podemos estar más de acuerdo con usted. Excelente respuesta!
Bonifacio
23
El código demasiado complejo también es bastante pasivo-agresivo. Está produciendo deliberadamente código que pocos otros pueden leer o depurar fácilmente ... lo que significa seguridad laboral para usted y el infierno para todos los demás en su ausencia. También puede escribir su documentación técnica completamente en latín.
Ivan
14
No creo que el código inteligente siempre sea una cosa para presumir. A veces se siente natural, y solo se ve ridículo en una segunda inspección.
55
He eliminado la frase sobre "presumir", ya que sonaba más crítico de lo previsto.
JacquesB
11
@BaileyS: creo que eso enfatiza la importancia de la revisión del código; lo que se siente natural y directo para el codificador, especialmente cuando se desarrolla gradualmente de esa manera, puede parecer complicado para un revisor. El código luego no pasa la revisión hasta que se refactoriza / reescribe para eliminar la convolución.
Myles
21

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.

b0nyb0y
fuente
2
+1 por señalar esta actitud generalizada (no necesariamente se aplica al OP): "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 ".
Giorgio
+1. Solo una pequeña observación pedante, puede usar el operador de propagación (...) en lugar de _.concat para eliminar esa dependencia.
YoTengoUnLCD
1
@YoTengoUnLCD Ah, buena captura. Ahora sabe que mi equipo y yo todavía estamos en el viaje de desaprender algunos de nuestros lodashusos. Ese código puede usarse spread operator, o incluso [].concat()si uno quiere mantener intacta la forma del código.
b0nyb0y
Lo siento, pero este listado de códigos es mucho menos obvio para mí que el "código junior" original en la publicación de OP. Básicamente: nunca use el operador ternario si puede evitarlo. Es muy tenso. En un lenguaje funcional "real", las declaraciones if serían expresiones y no declaraciones y, por lo tanto, más legibles.
Olle Härstedt
@ OlleHärstedt Umm, esa es una afirmación interesante que hiciste. La cuestión es que el paradigma de la programación funcional, o cualquier paradigma existente, nunca está vinculado a ningún lenguaje funcional "real" particular, y mucho menos a su sintaxis. Por lo tanto, dictar qué construcciones condicionales deben ser o no "nunca" deben usarse no tiene ningún sentido. A ternary operatores tan válido como una ifdeclaración regular , te guste o no. El debate sobre la legibilidad entre if-elsey el ?:campamento es interminable, por lo que no entraré en él. Todo lo que diré es que, con ojos entrenados, líneas como estas son apenas "demasiado tensas".
b0nyb0y
5

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.

Dawood ibn Kareem
fuente
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. 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).

user949300
fuente