Czy jestem zbyt „sprytny”, aby być czytelnym dla Jr. devs? Za dużo programowania funkcjonalnego w moim JS? [Zamknięte]

Jestem programistą front-end Sr., koduję w Babel ES6. Część naszej aplikacji wykonuje wywołanie API, a na podstawie modelu danych, który otrzymujemy z wywołania API, niektóre formularze muszą zostać wypełnione.

Te formularze są przechowywane na podwójnie połączonej liście (jeśli zaplecze mówi, że niektóre dane są nieprawidłowe, możemy szybko przywrócić użytkownika do jednej strony, którą pomieszali, a następnie przywrócić je do celu, po prostu modyfikując lista.)

W każdym razie istnieje wiele funkcji służących do dodawania stron i zastanawiam się, czy nie jestem zbyt sprytny. To tylko podstawowy przegląd - faktyczny algorytm jest znacznie bardziej złożony, z mnóstwem różnych stron i typów stron, ale to da ci przykład.

Myślę, że tak poradziłby sobie początkujący programista.

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

Teraz, aby być bardziej testowalnym, wziąłem wszystkie te instrukcje if i rozdzieliłem je, samodzielne funkcje, a następnie mapuję je.

Teraz testowalność to jedna rzecz, ale tak też jest czytelna i zastanawiam się, czy nie sprawiam, że rzeczy są mniej czytelne.

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

}

Oto moja troska. Dla mnie dno jest bardziej zorganizowane. Sam kod jest podzielony na mniejsze części, które można testować w izolacji. ALE myślę: gdybym musiał to przeczytać jako młodszy programista, nieprzyzwyczajony do takich pojęć, jak używanie funktorów tożsamości, curry lub potrójne stwierdzenia, czy byłbym w stanie zrozumieć, co robi to drugie rozwiązanie? Czy czasem lepiej jest robić rzeczy w „zły, łatwiejszy” sposób?

W swoim kodzie wprowadziłeś wiele zmian:

• przypisanie destrukcyjne dostępu do pól w pagesjest dobrą zmianą.
• wyodrębnienie parseFoo()funkcji itp. jest prawdopodobnie dobrą zmianą.
• wprowadzenie funktora jest… bardzo mylące.

Jedną z najbardziej zagmatwanych części jest tutaj mieszanie programowania funkcjonalnego i imperatywnego. Za pomocą funktora tak naprawdę nie transformujesz danych, używasz go do przepuszczania listy zmiennych przez różne funkcje. To nie wydaje się bardzo przydatną abstrakcją, mamy już do tego zmienne. To, co prawdopodobnie powinno być wyabstrahowane - analizowanie tylko tego elementu, jeśli istnieje - jest nadal wyraźnie w twoim kodzie, ale teraz musimy pomyśleć za rogiem. Na przykład, nie jest oczywiste, że parseFoo(foo)funkcja zwróci. JavaScript nie ma systemu typu statycznego, który powiadamiałby cię, czy jest to legalne, więc taki kod jest naprawdę podatny na błędy bez lepszej nazwy ( makeFooParser(foo)?). Nie widzę żadnej korzyści z tego zaciemnienia.

Co zamiast tego spodziewałbym się zobaczyć:

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

Ale to też nie jest idealne, ponieważ ze strony połączeń nie jest jasne, że elementy zostaną dodane do listy stron. Jeśli zamiast tego funkcje analizujące są czyste i zwracają (ewentualnie pustą) listę, którą możemy jawnie dodać do stron, może być lepiej:

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

Dodatkowa korzyść: logika dotycząca tego, co zrobić, gdy element jest pusty, została teraz przeniesiona do poszczególnych funkcji parsowania. Jeśli nie jest to właściwe, nadal możesz wprowadzić warunki warunkowe. Zmienność pageslisty jest teraz połączona w jedną funkcję, zamiast rozłożyć ją na wiele wywołań. Unikanie mutacji nielokalnych jest znacznie większą częścią programowania funkcjonalnego niż abstrakcje o śmiesznych nazwach Monad.

Więc tak, twój kod był zbyt sprytny. Proszę, zastosujcie swoją spryt, aby nie pisać sprytnego kodu, ale aby znaleźć sprytne sposoby uniknięcia potrzeby rażącej sprytności. Najlepsze projekty nie wyglądają fantazyjnie, ale są oczywiste dla każdego, kto je zobaczy. A dobre abstrakcje mają uprościć programowanie, a nie dodawać dodatkowych warstw, które muszę najpierw rozplątać w umyśle (tutaj, stwierdzając, że funktor jest równoważny zmiennej i może być skutecznie pomijany).

Proszę: w razie wątpliwości utrzymuj swój kod prosty i głupi (zasada KISS).

Jeśli masz wątpliwości, prawdopodobnie jest to zbyt sprytne! Drugi przykład wprowadza przypadkową złożoność z podobnymi wyrażeniami foo ? parseFoo(foo) : x => x, a ogólnie kod jest bardziej złożony, co oznacza, że ​​trudniej jest go śledzić.

Rzekoma korzyść polegająca na tym, że można testować poszczególne porcje, można osiągnąć w prostszy sposób, po prostu dzieląc się na poszczególne funkcje. W drugim przykładzie łączysz oddzielne iteracje, dzięki czemu masz mniej izolacji.

Bez względu na to, jakie są twoje odczucia dotyczące funkcjonalnego stylu, jest to wyraźnie przykład, w którym kod jest bardziej złożony.

Trochę ostrzegam, że kojarzysz prosty i bezpośredni kod z „nowicjuszami”. To niebezpieczna mentalność. Z mojego doświadczenia wynika, że ​​jest odwrotnie: nowi programiści są podatni na zbyt skomplikowany i sprytny kod, ponieważ potrzeba więcej doświadczenia, aby móc zobaczyć najprostsze i najczystsze rozwiązanie.

Odradzanie „sprytnego kodu” tak naprawdę nie dotyczy tego, czy kod wykorzystuje zaawansowane koncepcje, których nowicjusz może nie zrozumieć. Chodzi raczej o pisanie kodu, który jest bardziej złożony lub skomplikowany niż to konieczne . To sprawia, że ​​kod jest trudniejszy do naśladowania dla wszystkich , zarówno dla nowicjuszy, jak i ekspertów, a prawdopodobnie również dla ciebie kilka miesięcy później.

Moja odpowiedź jest nieco spóźniona, ale nadal chcę wejść. Tylko dlatego, że używasz najnowszych technik ES6 lub najbardziej popularnego paradygmatu programistycznego, niekoniecznie oznacza, że ​​twój kod jest poprawniejszy lub kod tego młodszego jest źle. Funkcjonalne programowanie (lub dowolną inną technikę) powinno być stosowane wtedy, gdy jest rzeczywiście potrzebne. Jeśli spróbujesz znaleźć najmniejszą szansę na wbicie najnowszych technik programowania w każdy problem, zawsze znajdziesz rozwiązanie o zbyt wysokim poziomie inżynierii.

Cofnij się o krok i spróbuj zwerbalizować problem, który próbujesz rozwiązać przez sekundę. W skrócie, po prostu chcesz, addPagesaby funkcja przekształciła różne części apiDataw zestaw par klucz-wartość, a następnie dodaj je wszystkie PagesList.

A jeśli to wszystko jest do niego, dlaczego przeszkadza korzystania identity functionz ternary operator, lub używając functordo wejścia parsowania? Poza tym, dlaczego uważasz, że jest to właściwe podejście, functional programmingktóre powoduje skutki uboczne (poprzez mutowanie listy)? Dlaczego wszystkie te rzeczy, kiedy wszystko czego potrzebujesz to tylko:

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

( tutaj można uruchomić jsfiddle )

Jak widać, to podejście nadal stosuje functional programming, ale z umiarem. Należy również pamiętać, że ponieważ wszystkie 3 funkcje transformacji nie powodują żadnych skutków ubocznych, są one łatwe do przetestowania. Kod addPagesjest również trywialny i niepozorny, który nowicjusze lub eksperci mogą zrozumieć tylko jednym spojrzeniem.

Teraz porównaj ten kod z tym, co wymyśliłeś powyżej, czy widzisz różnicę? Niewątpliwie, functional programminga składnie ES6 są potężne, ale jeśli rozwiążesz problem w niewłaściwy sposób za pomocą tych technik, skończysz z jeszcze bardziej nieporządnym kodem.

Jeśli nie wpadniesz w problem i nie zastosujesz odpowiednich technik we właściwych miejscach, możesz mieć kod, który ma funkcjonalny charakter, a jednocześnie jest bardzo zorganizowany i łatwy do utrzymania dla wszystkich członków zespołu. Te cechy nie wykluczają się wzajemnie.

Drugi fragment kodu nie jest bardziej testowalny niż pierwszy. Zrozumiałe byłoby skonfigurowanie wszystkich niezbędnych testów dla jednego z dwóch fragmentów.

Prawdziwą różnicą między tymi dwoma fragmentami jest zrozumiałość. Mogę dość szybko przeczytać pierwszy fragment i zrozumieć, co się dzieje. Drugi fragment, nie tyle. Jest o wiele mniej intuicyjny, a także znacznie dłuższy.

Dzięki temu pierwszy fragment kodu jest łatwiejszy w utrzymaniu, co stanowi cenną jakość kodu. W drugim fragmencie znajduję niewiele wartości.

TD; DR

1. Czy potrafisz wyjaśnić swój kod Junior Developer w 10 minut lub krócej?
2. Za dwa miesiące możesz zrozumieć swój kod?

Szczegółowa analiza

Jasność i czytelność

Oryginalny kod jest imponująco przejrzysty i łatwy do zrozumienia dla każdego poziomu programisty. Jest w stylu znanym wszystkim .

Czytelność opiera się w dużej mierze na znajomości, a nie na matematycznym liczeniu tokenów . IMO, na tym etapie masz za dużo ES6 w przepisywaniu. Może za kilka lat zmienię tę część mojej odpowiedzi. :-) BTW, podoba mi się również odpowiedź @ b0nyb0y jako rozsądny i jasny kompromis.

Testowalność

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

Zakładając, że PagesList.add () ma testy, które powinny, jest to całkowicie prosty kod i nie ma oczywistego powodu, aby ta sekcja wymagała specjalnego oddzielnego testowania.

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

Ponownie nie widzę potrzeby natychmiastowego przeprowadzania specjalnych oddzielnych testów tej sekcji. Chyba że PagesList.add () ma nietypowe problemy z zerami lub duplikatami lub innymi danymi wejściowymi.

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

Ten kod jest również bardzo prosty. Zakładając, że customBazParserjest przetestowany i nie zwraca zbyt wielu „specjalnych” wyników. Więc znowu, chyba że występują trudne sytuacje z `PagesList.add (), (które mogą być, ponieważ nie znam twojej domeny). Nie rozumiem, dlaczego ta sekcja wymaga specjalnych testów.

Ogólnie rzecz biorąc, testowanie całej funkcji powinno działać dobrze.

Zastrzeżenie : Jeśli istnieją specjalne powody, aby przetestować wszystkie 8 możliwości trzech if()instrukcji, to tak, podziel testy. Lub, jeśli PagesList.add()jest wrażliwy, tak, podziel testy.

Struktura: Czy warto podzielić na trzy części (jak Gal)

Tutaj masz najlepszy argument. Osobiście nie sądzę, aby oryginalny kod był „zbyt długi” (nie jestem fanatykiem SRP). Ale gdyby było jeszcze kilka if (apiData.pages.blah)sekcji, SRP zbiera brzydką głowę i warto byłoby podzielić. Zwłaszcza jeśli ma zastosowanie DRY, a funkcji można użyć w innych miejscach kodu.

Moja jedna sugestia

YMMV. Aby zapisać wiersz kodu i logikę, mógłbym połączyć if i pozwolić na jedną linię: np

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

To się nie powiedzie, jeśli apiData.pages.arrayOfBars jest liczbą lub łańcuchem, ale również oryginalny kod. I dla mnie jest to wyraźniejsze (i nadużywany idiom).