Bin ich zu "schlau", um von jr. Entwicklern gelesen zu werden? Zu viel funktionale Programmierung in meinem JS? [geschlossen]

Ich bin ein Senior-Front-End-Entwickler und programmiere in Babel ES6. Ein Teil unserer App führt einen API-Aufruf durch und basierend auf dem Datenmodell, das wir vom API-Aufruf erhalten, müssen bestimmte Formulare ausgefüllt werden.

Diese Formulare werden in einer doppelt verknüpften Liste gespeichert (wenn das Back-End angibt, dass einige der Daten ungültig sind, können wir den Benutzer schnell zu der einen Seite zurückbringen, auf der er sie durcheinander gebracht hat, und sie dann wieder auf das Ziel bringen, indem Sie einfach die ändern Liste.)

Wie auch immer, es gibt eine Reihe von Funktionen zum Hinzufügen von Seiten, und ich frage mich, ob ich zu schlau bin. Dies ist nur eine grundlegende Übersicht - der eigentliche Algorithmus ist viel komplexer, mit Tonnen verschiedener Seiten und Seitentypen, aber dies wird Ihnen ein Beispiel geben.

Ich denke, ein Anfänger würde so damit umgehen.

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

Um die Testbarkeit zu verbessern, habe ich all diese if-Anweisungen aufgenommen und sie getrennt, eigenständige Funktionen erstellt und sie dann gemappt.

Nun, testbar ist eine Sache, aber auch lesbar, und ich frage mich, ob ich die Dinge hier weniger lesbar mache.

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

}

Hier ist mein Anliegen. Für mich ist der Boden organisierter. Der Code selbst ist in kleinere Abschnitte unterteilt, die isoliert getestet werden können. ABER ich denke: Wenn ich das als Junior-Entwickler lesen müsste, der nicht mit Konzepten wie Identitätsfunktionen, Currys oder ternären Anweisungen vertraut ist, würde ich überhaupt verstehen können, was die letztere Lösung tut? Ist es manchmal besser, die Dinge "falsch, einfacher" zu machen?

In Ihrem Code haben Sie mehrere Änderungen vorgenommen:

• Die Zuordnung zu Zugriffsfeldern in der pagesist eine gute Änderung.
• Das Extrahieren der parseFoo()Funktionen usw. ist möglicherweise eine gute Änderung.
• Die Einführung eines Funktors ist… sehr verwirrend.

Einer der verwirrendsten Aspekte dabei ist, wie Sie funktionale und imperative Programmierung mischen. Mit Ihrem Funktor transformieren Sie Daten nicht wirklich, sondern verwenden sie, um eine veränderbare Liste durch verschiedene Funktionen zu führen. Das scheint keine sehr nützliche Abstraktion zu sein, wir haben bereits Variablen dafür. Die Sache, die möglicherweise hätte abstrahiert werden sollen - nur das Parsen dieses Elements, wenn es existiert - ist noch explizit in Ihrem Code vorhanden, aber jetzt müssen wir um die Ecke denken. Beispielsweise ist es nicht offensichtlich, dass parseFoo(foo)eine Funktion zurückgegeben wird. JavaScript verfügt nicht über ein statisches Typsystem, mit dem Sie benachrichtigt werden, ob dies zulässig ist. Daher ist ein solcher Code ohne einen besseren Namen ( makeFooParser(foo)?) Sehr fehleranfällig . Ich sehe keinen Nutzen in dieser Verschleierung.

Was ich stattdessen erwarten würde:

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

Dies ist jedoch auch nicht ideal, da auf der Anrufseite nicht ersichtlich ist, dass die Elemente der Seitenliste hinzugefügt werden. Wenn die Analysefunktionen stattdessen rein sind und eine (möglicherweise leere) Liste zurückgeben, die wir explizit zu den Seiten hinzufügen können, ist dies möglicherweise besser:

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

Zusätzlicher Vorteil: Die Logik, was zu tun ist, wenn der Artikel leer ist, wurde jetzt in die einzelnen Analysefunktionen verschoben. Wenn dies nicht angemessen ist, können Sie dennoch Bedingungen einführen. Die Änderbarkeit der pagesListe wird jetzt zu einer einzigen Funktion zusammengefasst, anstatt sie auf mehrere Aufrufe zu verteilen. Nicht-lokale Mutationen zu vermeiden, ist ein weitaus größerer Teil der funktionalen Programmierung als Abstraktionen mit lustigen Namen wie Monad.

Also ja, dein Code war zu schlau. Bitte wenden Sie Ihre Cleverness an, um keinen cleveren Code zu schreiben, sondern um clevere Wege zu finden, um die Notwendigkeit offensichtlicher Cleverness zu vermeiden. Die besten Entwürfe nicht aussehen Phantasie, sondern für jeden, der sie offensichtlich aussehen sieht. Und gute Abstraktionen dienen dazu, die Programmierung zu vereinfachen und keine zusätzlichen Ebenen hinzuzufügen, die ich zuerst entwirren muss (hier wird herausgefunden, dass der Funktor einer Variablen entspricht und effektiv entfernt werden kann).

Bitte: Halten Sie im Zweifelsfall Ihren Code einfach und dumm (KISS-Prinzip).

Wenn Sie Zweifel haben, ist es wahrscheinlich zu klug! Das zweite Beispiel führt eine zufällige Komplexität mit Ausdrücken wie ein foo ? parseFoo(foo) : x => x, und insgesamt ist der Code komplexer, was bedeutet, dass es schwieriger ist, zu folgen.

Der vorgebliche Vorteil, dass Sie die Chunks einzeln testen können, könnte auf einfachere Weise durch bloßes Aufbrechen in einzelne Funktionen erreicht werden. Und im zweiten Beispiel koppeln Sie die ansonsten getrennten Iterationen, sodass Sie tatsächlich weniger isoliert sind.

Unabhängig davon, wie Sie den funktionalen Stil im Allgemeinen einschätzen, ist dies eindeutig ein Beispiel dafür, wie der Code komplexer wird.

Ein kleines Warnsignal ist, dass Sie einfachen und unkomplizierten Code mit "unerfahrenen Entwicklern" verknüpfen. Das ist eine gefährliche Mentalität. Nach meiner Erfahrung ist das Gegenteil der Fall: Anfänger sind anfällig für übermäßig komplexen und cleveren Code, da mehr Erfahrung erforderlich ist, um die einfachste und klarste Lösung zu finden.

Der Rat gegen "cleveren Code" handelt nicht wirklich davon, ob der Code fortgeschrittene Konzepte verwendet oder nicht, die ein Anfänger vielleicht nicht versteht. Es geht vielmehr darum, Code zu schreiben, der komplexer oder komplizierter als nötig ist . Dies erschwert es jedem , Anfängern und Experten gleichermaßen, und wahrscheinlich auch Ihnen selbst , den Code einige Monate später zu befolgen .

Diese Antwort von mir kommt ein bisschen spät, aber ich möchte mich trotzdem einschalten. Nur weil Sie die neuesten ES6-Techniken oder das beliebteste Programmierparadigma verwenden, bedeutet dies nicht zwangsläufig, dass Ihr Code korrekter ist oder der Code dieses Juniors ist falsch. Funktionale Programmierung (oder eine andere Technik) sollte verwendet werden, wenn sie tatsächlich benötigt wird. Wenn Sie versuchen, die kleinste Chance zu finden, die neuesten Programmiertechniken in jedes Problem zu stecken, haben Sie immer eine überentwickelte Lösung.

Machen Sie einen Schritt zurück und versuchen Sie, das Problem, das Sie lösen möchten, für eine Sekunde zu formulieren. Im Wesentlichen möchten Sie lediglich, dass eine Funktion addPagesverschiedene Teile von apiDatain eine Reihe von Schlüssel-Wert-Paaren umwandelt und diese dann alle hinzufügt PagesList.

Und wenn das alles ist, warum sollte man sich die Mühe machen, identity functionmit ternary operatoroder functorzum Parsen von Eingaben zu verwenden? Außerdem, warum denken Sie , auch ein richtiger Ansatz ist es anzuwenden , functional programmingdie bewirkt , dass Nebenwirkungen (durch die Liste mutiert)? Warum all diese Dinge, wenn Sie nur Folgendes brauchen:

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

(ein lauffähiges jsfiddle hier )

Wie Sie sehen, wird dieser Ansatz immer noch verwendet functional programming, jedoch in Maßen. Beachten Sie auch, dass alle 3 Transformationsfunktionen keinerlei Nebenwirkungen verursachen und daher kinderleicht zu testen sind. Der Code in addPagesist auch trivial und bescheiden, dass Anfänger oder Experten durch nur einen Blick verstehen können.

Vergleichen Sie nun diesen Code mit dem, was Sie oben gefunden haben. Sehen Sie den Unterschied? Zweifellos functional programmingund ES6-Syntaxen sind mächtig, aber wenn Sie das Problem mit diesen Techniken falsch aufteilen, erhalten Sie sogar noch chaotischeren Code.

Wenn Sie sich nicht mit dem Problem befassen und die richtigen Techniken an den richtigen Stellen anwenden, können Sie den Code verwenden, der von Natur aus funktionsfähig ist, aber dennoch von allen Teammitgliedern gut organisiert und gewartet werden kann. Diese Eigenschaften schließen sich nicht aus.

Das zweite Snippet ist nicht mehr testbar als das erste. Es wäre ziemlich einfach, alle erforderlichen Tests für eines der beiden Snippets einzurichten.

Der wahre Unterschied zwischen den beiden Ausschnitten ist die Verständlichkeit. Ich kann das erste Snippet ziemlich schnell lesen und verstehen, was los ist. Der zweite Ausschnitt, nicht so sehr. Es ist viel weniger intuitiv und wesentlich länger.

Dies erleichtert die Pflege des ersten Snippets, was eine wertvolle Qualität des Codes darstellt. Ich finde sehr wenig Wert im zweiten Ausschnitt.

TD; DR

1. Können Sie dem Junior-Entwickler Ihren Code in maximal 10 Minuten erklären?
2. Können Sie in zwei Monaten Ihren Code verstehen?

Detaillierte Analyse

Klarheit und Lesbarkeit

Der ursprüngliche Code ist beeindruckend klar und für jeden Programmierer leicht verständlich. Es ist in einem Stil , den jeder kennt .

Die Lesbarkeit basiert größtenteils auf Vertrautheit, nicht auf mathematischem Zählen von Token . IMO, zu diesem Zeitpunkt haben Sie zu viel ES6 in Ihrem Umschreiben. Vielleicht ändere ich in ein paar Jahren diesen Teil meiner Antwort. :-) Übrigens gefällt mir auch die @ b0nyb0y-Antwort als vernünftiger und klarer Kompromiss.

Testbarkeit

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

Angenommen, PagesList.add () verfügt über Tests, die es sollte, dann ist dies völlig unkomplizierter Code, und es gibt keinen offensichtlichen Grund dafür, dass dieser Abschnitt spezielle separate Tests benötigt.

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

Auch hier sehe ich keine unmittelbare Notwendigkeit für eine gesonderte Prüfung dieses Abschnitts. Es sei denn, PagesList.add () weist ungewöhnliche Probleme mit Nullen oder Duplikaten oder anderen Eingaben auf.

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

Dieser Code ist auch sehr einfach. Vorausgesetzt, das customBazParserist getestet und liefert nicht zu viele "spezielle" Ergebnisse. Also noch einmal, es sei denn, es gibt schwierige Situationen mit `PagesList.add () (die auftreten können, da ich mit Ihrer Domain nicht vertraut bin). Ich verstehe nicht, warum in diesem Abschnitt spezielle Tests erforderlich sind.

Im Allgemeinen sollte das Testen der gesamten Funktion problemlos funktionieren.

Haftungsausschluss : Wenn es besondere Gründe gibt, alle 8 Möglichkeiten der drei if()Aussagen zu testen , teilen Sie die Tests auf. Wenn PagesList.add()es empfindlich ist, teilen Sie die Tests auf.

Struktur: Lohnt es sich in drei Teile zu zerlegen (wie Gallien)

Hier hast du das beste Argument. Persönlich denke ich nicht, dass der ursprüngliche Code "zu lang" ist (ich bin kein SRP-Fan). Aber wenn es noch ein paar if (apiData.pages.blah)Abschnitte gäbe , dann wäre SRP hässlich und es wäre eine Aufteilung wert. Insbesondere, wenn DRY zutrifft und die Funktionen an anderen Stellen des Codes verwendet werden könnten.

Mein einziger Vorschlag

YMMV. Um eine Codezeile und eine Logik zu speichern, könnte ich das if und let in eine Zeile kombinieren : zB

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

Dies schlägt fehl, wenn apiData.pages.arrayOfBars eine Zahl oder eine Zeichenfolge ist, der ursprüngliche Code jedoch auch. Und für mich ist es klarer (und eine überstrapazierte Redewendung).