Sono troppo intelligente per essere leggibile dagli sviluppatori Jr.? Troppa programmazione funzionale nel mio JS? [chiuso]

131

Sono uno sviluppatore di front-end Sr., che codifica in Babel ES6. Parte della nostra app effettua una chiamata API e, in base al modello di dati che riusciamo a recuperare dalla chiamata API, alcuni moduli devono essere compilati.

Questi moduli sono memorizzati in una lista doppiamente collegata (se il back-end dice che alcuni dati non sono validi, possiamo riportare rapidamente l'utente alla pagina che hanno incasinato e poi riportarli sul target, semplicemente modificando la lista.)

Ad ogni modo, ci sono un sacco di funzioni usate per aggiungere pagine, e mi chiedo se sono troppo intelligente. Questa è solo una panoramica di base: l'algoritmo attuale è molto più complesso, con tonnellate di pagine e tipi di pagine diversi, ma questo ti fornirà un esempio.

Questo è il modo in cui, penso, un programmatore principiante potrebbe gestirlo.

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

Ora, per essere più testabile, ho preso tutte quelle dichiarazioni if e le ho rese separate, funzioni autonome, e quindi ho mappato su di esse.

Ora, testabile è una cosa, ma lo è anche leggibile e mi chiedo se sto rendendo le cose meno leggibili qui.

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

}

Ecco la mia preoccupazione. Per me il fondo è più organizzato. Il codice stesso è suddiviso in blocchi più piccoli che sono testabili separatamente. Ma sto pensando: se dovessi leggerlo come uno sviluppatore junior, non abituato a concetti come l'utilizzo di funtori Identity, curry o dichiarazioni ternarie, sarei in grado di capire anche cosa sta facendo la seconda soluzione? È meglio fare cose "sbagliate, più facili" a volte?

    
posta Brian Boyko 06.06.2017 - 06:52
fonte

5 risposte

320

Nel tuo codice hai apportato più modifiche:

  • l'assegnazione distruttiva dei campi di accesso in pages è un buon cambiamento.
  • estrarre le funzioni parseFoo() , ecc. è probabilmente un buon cambiamento.
  • l'introduzione di un functor è ... molto confuso.

Una delle parti più confuse qui è come si mischia la programmazione funzionale e imperativa. Con il tuo functor non stai veramente trasformando i dati, lo stai usando per passare una lista mutabile attraverso varie funzioni. Non sembra un'astrazione molto utile, abbiamo già delle variabili per questo. La cosa che dovrebbe essere stata sottratta - solo analizzando quell'elemento se esiste - è ancora presente nel tuo codice esplicitamente, ma ora dobbiamo pensare dietro l'angolo. Ad esempio, è alquanto ovvio che parseFoo(foo) restituirà una funzione. JavaScript non ha un sistema di tipo statico per notificare se questo è legale, quindi tale codice è in realtà soggetto a errori senza un nome migliore ( makeFooParser(foo) ?). Non vedo alcun beneficio in questa offuscazione.

Ciò che mi aspetto di vedere invece:

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

Ma non è nemmeno l'ideale, perché dal sito di chiamata non è chiaro che gli articoli saranno aggiunti all'elenco delle pagine. Se invece le funzioni di analisi sono pure e restituiscono una lista (eventualmente vuota) che possiamo aggiungere esplicitamente alle pagine, potrebbe essere meglio:

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

Vantaggio aggiunto: la logica su cosa fare quando l'elemento è vuoto è stata ora spostata nelle singole funzioni di analisi. Se ciò non è appropriato, puoi comunque introdurre condizionali. La mutabilità della lista pages è ora riunita in una singola funzione, invece di diffonderla tra più chiamate. Evitare le mutazioni non locali è una parte molto più grande della programmazione funzionale rispetto alle astrazioni con nomi divertenti come Monad .

Quindi sì, il tuo codice era troppo intelligente. Per favore applica la tua intelligenza a non scrivere codice intelligente, ma a trovare modi intelligenti per evitare il bisogno di un'astuzia clamorosa. I migliori design non sono look fantasiosi, ma sembrano ovvi per chiunque li veda. E buone astrazioni ci sono per semplificare la programmazione, non per aggiungere strati extra che devo prima districare nella mia mente (qui, capire che il functor è equivalente a una variabile, e può essere efficacemente eluito).

Per favore: se sei in dubbio, mantieni il tuo codice semplice e stupido (principio KISS).

    
risposta data 06.06.2017 - 08:43
fonte
223

Se sei in dubbio, probabilmente è troppo intelligente! Il secondo esempio introduce complessità accidentale con espressioni come foo ? parseFoo(foo) : x => x , e nel complesso il codice è più complesso, il che significa che è più difficile da seguire.

Il presunto vantaggio, che è possibile testare singolarmente i blocchi, potrebbe essere ottenuto in un modo più semplice, interrompendo semplicemente le singole funzioni. E nel secondo esempio accoppi le iterazioni altrimenti separate, in modo da ottenere un meno isolamento.

Qualunque sia la tua opinione sullo stile funzionale in generale, questo è chiaramente un esempio in cui rende il codice più complesso.

Trovo un po 'un segnale di avviso in quanto associ codice semplice e diretto con "sviluppatori novizi". Questa è una mentalità pericolosa. Nella mia esperienza è il contrario: gli sviluppatori novizi sono inclini a codice troppo complesso e intelligente, perché richiede più esperienza per essere in grado di vedere la soluzione più semplice e chiara.

Il consiglio contro il "codice intelligente" non riguarda in realtà se il codice utilizza o meno concetti avanzati che un novizio potrebbe non capire. Piuttosto si tratta di scrivere codice che è più complesso o complicato del necessario . Ciò rende il codice più difficile da seguire per tutti , sia per i novizi che per gli esperti, e probabilmente anche per te stesso, alcuni mesi dopo il termine.

    
risposta data 06.06.2017 - 08:45
fonte
20

Questa mia risposta arriva un po 'in ritardo, ma voglio ancora intervenire. Solo perché stai usando le più recenti tecniche ES6 o utilizzando il paradigma di programmazione più popolare non significa necessariamente che il tuo codice sia più corretto, o il codice di quel junior è sbagliato La programmazione funzionale (o qualsiasi altra tecnica) dovrebbe essere utilizzata quando è effettivamente necessaria. Se si cerca di trovare la minima possibilità di assimilare le ultime tecniche di programmazione in ogni problema, si finisce sempre con una soluzione troppo ingegnerizzata.

Fai un passo indietro e prova a verbalizzare il problema che stai cercando di risolvere per un secondo. In sostanza, vuoi solo una funzione addPages per trasformare parti diverse di apiData in un insieme di coppie chiave-valore, quindi aggiungile tutte in PagesList .

E se è tutto lì, perché preoccuparsi di usare identity function con ternary operator o usare functor per l'analisi di input? Inoltre, perché pensi che sia un approccio corretto applicare functional programming che causa effetti collaterali ( mutando la lista)? Perché tutte quelle cose, quando tutto ciò di cui hai bisogno è solo questo:

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 eseguibile qui )

Come puoi vedere, questo approccio utilizza ancora functional programming , ma con moderazione. Si noti inoltre che poiché tutte e 3 le funzioni di trasformazione non causano alcun effetto collaterale, sono comunque facili da testare. Anche il codice in addPages è banale e senza pretese che i novizi o gli esperti possono capire solo con un semplice sguardo.

Ora, confronta questo codice con quello che hai inventato sopra, vedi la differenza? Indubbiamente, le sintassi di functional programming e ES6 sono potenti, ma se si affronta il problema in modo sbagliato con queste tecniche, si finisce con un codice ancora più incasinato.

Se non affronti il problema, e applicando le giuste tecniche nei posti giusti, puoi avere il codice che è funzionale in natura, mentre è ancora molto organizzato e manutenibile da tutti i membri del team. Queste caratteristiche non si escludono a vicenda.

    
risposta data 09.06.2017 - 21:30
fonte
5

Il secondo snippet è non più testabile del primo. Sarebbe ragionevolmente semplice impostare tutti i test necessari per uno dei due snippet.

La vera differenza tra i due frammenti è la comprensibilità. Posso leggere il primo frammento abbastanza velocemente e capire cosa sta succedendo. Il secondo frammento, non tanto. È molto meno intuitivo e sostanzialmente più lungo.

Ciò rende il primo frammento di codice più facile da mantenere, che è una preziosa qualità del codice. Trovo molto poco di valore nel secondo snippet.

    
risposta data 10.06.2017 - 09:45
fonte
3

TD; DR

  1. Puoi spiegare il tuo codice allo sviluppatore minore in 10 minuti o meno?
  2. Tra due mesi, puoi capire il tuo codice?

Analisi dettagliata

Chiarezza e leggibilità

Il codice originale è straordinariamente chiaro e facile da capire per qualsiasi livello di programmatore. È in uno stile familiare a tutti .

La leggibilità è in gran parte basata sulla familiarità, non su un conteggio matematico di token . IMO, in questo momento, hai troppo ES6 nella tua riscrittura. Forse tra un paio d'anni cambierò questa parte della mia risposta. :-) A proposito, mi piace anche la risposta @ b0nyb0y come un compromesso ragionevole e chiaro.

Testabilità

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

Supponendo che PagesList.add () abbia dei test, quale dovrebbe, questo è un codice completamente semplice e non vi è alcun motivo ovvio per questa sezione di aver bisogno di speciali test separati.

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

Ancora una volta, non vedo l'immediata necessità di test speciali separati di questa sezione. A meno che PagesList.add () non abbia problemi insoliti con valori null o duplicati o altri input.

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

Questo codice è anche molto semplice. Supponendo che customBazParser sia testato e non restituisca troppi risultati "speciali". Quindi, a meno che non ci siano situazioni complicate con 'PagesList.add (), (che potrebbe esserci dato che non ho familiarità con il tuo dominio) non vedo perché questa sezione abbia bisogno di test speciali.

In generale, testare l'intera funzione dovrebbe funzionare correttamente.

Dichiarazione di non responsabilità : se ci sono motivi particolari per testare tutte le 8 possibilità delle tre dichiarazioni if() , allora sì, dividi i test. Oppure, se PagesList.add() è sensibile, sì, dividi i test.

Struttura: vale la pena dividere in tre parti (come Gallia)

Ecco la migliore argomentazione. Personalmente, non penso che il codice originale sia "troppo lungo" (non sono un fanatico della SRP). Ma, se ci fossero alcune altre sezioni if (apiData.pages.blah) , SRP alza la sua brutta testa e varrebbe la pena dividerlo. Soprattutto se si applica DRY e le funzioni potrebbero essere utilizzate in altre parti del codice.

Il mio suggerimento

YMMV. Per salvare una riga di codice e un po 'di logica, potrei combinare if e let in una riga: ad es.

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

Questo fallirà se apiData.pages.arrayOfBars è un numero o una stringa, ma lo sarà anche il codice originale. E per me è più chiaro (e un idioma abusato).

    
risposta data 10.06.2017 - 19:31
fonte