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

133

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?

Brian Boyko
quelle
13
Als jemand, der nur 10 Jahre Autodidakt in JS hat, würde ich mich als jr. bezeichnen und Sie haben mich umBabel ES6
Rozza
26
OMG - ist seit 1999 in der Industrie tätig und seit 1983 in der Codierung. Sie sind der schädlichste Entwickler, den es gibt. Was Sie für "clever" halten, wird als "teuer" und "schwer zu pflegen" und als "Fehlerquelle" bezeichnet und hat in einem Geschäftsumfeld keinen Platz. Das erste Beispiel ist einfach, leicht zu verstehen und funktioniert, während das zweite Beispiel komplex, schwer zu verstehen und nicht nachweislich korrekt ist. Bitte hören Sie auf, so etwas zu tun. Es ist NICHT besser, außer vielleicht in einem akademischen Sinne, der nicht auf die reale Welt zutrifft.
user1068
15
Ich möchte hier nur Brian Kerninghan zitieren: "Jeder weiß, dass das Debuggen doppelt so schwer ist wie das Schreiben eines Programms. Wenn Sie also so schlau sind, wie Sie können, wenn Sie es schreiben, wie werden Sie es jemals debuggen? " - de.wikiquote.org/wiki/Brian_Kernighan / "Die Elemente des Programmierstils", 2. Ausgabe, Kapitel 2.
MarkusSchaber 10.06.17
7
@Logister Coolness ist nicht mehr ein primäres Ziel als die Einfachheit. Der Einwand ist hier die unbegründete Komplexität, die den Feind der Korrektheit darstellt (sicherlich ein Hauptanliegen), da der Code schwieriger zu überlegen ist und mit größerer Wahrscheinlichkeit unerwartete Eckfälle enthält. Angesichts meiner bereits erwähnten Skepsis gegenüber Behauptungen, es sei tatsächlich einfacher zu testen, habe ich kein überzeugendes Argument für diesen Stil gesehen. In Analogie zur Regel des geringsten Privilegs für die Sicherheit könnte es vielleicht eine Faustregel geben, die besagt, dass man vorsichtig sein sollte, leistungsstarke Sprachfunktionen zu verwenden, um einfache Dinge zu tun.
Sdenham
6
Ihr Code sieht aus wie Junior-Code. Ich würde erwarten, dass ein Senior das erste Beispiel schreibt.
sed

Antworten:

322

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

amon
quelle
2
Was let pages = Identity(pagesList)unterscheidet sich aus symmetrischer Sicht von parseFoo(foo)? Da würde ich wahrscheinlich ... {Identity(pagesList), parseFoo(foo), parseBar(bar)}.flatMap(x -> x).
Kunst
8
Vielen Dank für die Erklärung, dass drei verschachtelte Lambda-Ausdrücke zum Sammeln einer zugeordneten Liste (für mein ungeübtes Auge) möglicherweise etwas zu clever sind.
Thorbjørn Ravn Andersen
2
Kommentare sind nicht für eine längere Diskussion gedacht. Diese Unterhaltung wurde in den Chat verschoben .
Yannis
Vielleicht würde ein fließender Stil in Ihrem zweiten Beispiel gut funktionieren?
user1068
225

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 .

JacquesB
quelle
156
"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", kann ich Ihnen nicht mehr zustimmen. Hervorragende Antwort!
Bonifacio
23
Übermäßig komplexer Code ist auch ziemlich passiv-aggressiv. Sie produzieren absichtlich Code, den nur wenige andere problemlos lesen oder debuggen können. Dies bedeutet für Sie Arbeitsplatzsicherheit und für alle anderen in Ihrer Abwesenheit die Hölle. Sie können Ihre technischen Unterlagen auch vollständig in lateinischer Sprache verfassen.
Ivan
14
Ich denke nicht, dass kluger Code immer eine Show ist. Manchmal fühlt es sich natürlich an und sieht erst bei einer zweiten Inspektion lächerlich aus.
5
Ich habe die Formulierung über "Angeberei" entfernt, da sie wertender klang als beabsichtigt.
JacquesB
11
@ BaileyS - Ich denke, das unterstreicht die Wichtigkeit der Codeüberprüfung; Was sich für den Programmierer natürlich und unkompliziert anfühlt, kann für einen Rezensenten leicht verworren erscheinen. Der Code wird dann erst überprüft, wenn er überarbeitet / neu geschrieben wurde, um die Faltung zu entfernen.
Myles
21

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.

b0nyb0y
quelle
2
+1 für den Hinweis auf diese weit verbreitete Haltung (gilt nicht unbedingt für das OP): "Nur weil Sie die neuesten ES6-Techniken oder das gängigste Programmierparadigma verwenden, bedeutet dies nicht zwangsläufig, dass Ihr Code korrekter ist. oder der Code dieses Juniors ist falsch. "
Giorgio
+1. Nur eine kleine pedantische Bemerkung, Sie können den spread (...) -Operator anstelle von _.concat verwenden, um diese Abhängigkeit zu entfernen.
YoTengoUnLCD
1
@YoTengoUnLCD Ah, guter Fang. Jetzt wissen Sie, dass ich und mein Team immer noch dabei sind, einen Teil unserer Arbeit zu verlernen lodash. Dieser Code kann verwendet spread operatorwerden oder auch, [].concat()wenn man die Form des Codes intakt halten möchte.
b0nyb0y
Entschuldigung, aber diese Codeauflistung ist für mich immer noch viel weniger offensichtlich als der ursprüngliche "Junior-Code" in OPs Beitrag. Grundsätzlich gilt: Verwenden Sie niemals ternäre Operatoren, wenn Sie dies vermeiden können. Es ist zu angespannt. In einer "echten" funktionalen Sprache wären if-Anweisungen Ausdrücke und keine Anweisungen und daher besser lesbar.
Olle Härstedt
@ OlleHärstedt Umm, das ist eine interessante Behauptung, die du aufgestellt hast. Die Sache ist, dass das funktionale Programmierparadigma oder irgendein Paradigma da draußen niemals an eine bestimmte "echte" funktionale Sprache gebunden ist, geschweige denn an ihre Syntax. Es macht also keinen Sinn zu diktieren, welche bedingten Konstrukte verwendet werden sollen oder "nie". A ternary operatorist so gültig wie eine reguläre ifAussage, ob Sie es mögen oder nicht. Die Lesbarkeitsdebatte zwischen if-elseund ?:Lager ist endlos, also werde ich nicht darauf eingehen. Alles, was ich sagen werde, ist, dass mit geschulten Augen Linien wie diese kaum "zu angespannt" sind.
b0nyb0y
5

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.

Dawood ibn Kareem
quelle
3

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

user949300
quelle