Я слишком «умен», чтобы быть доступным для чтения младшим разработчикам? Слишком много функционального программирования в моем JS? [закрыто]

Я старший разработчик, кодирую в Babel ES6. Часть нашего приложения выполняет вызов API, и в зависимости от модели данных, которую мы получаем от вызова API, необходимо заполнить определенные формы.

Эти формы хранятся в двусвязном списке (если серверная часть говорит, что некоторые данные недействительны, мы можем быстро вернуть пользователя на одну страницу, которую они испортили, а затем вернуть их на целевую страницу, просто изменив список.)

Во всяком случае, есть несколько функций, используемых для добавления страниц, и мне интересно, если я слишком умный. Это просто базовый обзор - фактический алгоритм намного сложнее, с множеством разных страниц и типов страниц, но это даст вам пример.

Вот как, я думаю, начинающий программист справится с этим.

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

Теперь, чтобы быть более тестируемым, я взял все эти операторы if и разделил их, отдельные функции, а затем сопоставил их.

Теперь, тестируемый - это одно, но он читабелен, и мне интересно, я делаю вещи менее читабельными здесь.

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

}

Вот моя забота. Для меня дно более организовано. Сам код разбит на более мелкие куски, которые можно тестировать изолированно. НО я думаю: если бы мне пришлось читать это как начинающий разработчик, не привыкший к таким понятиям, как использование функторов идентификации, карри или троичных выражений, смог бы я понять, что делает последнее решение? Лучше ли иногда делать вещи «неправильно, проще»?

В своем коде вы внесли несколько изменений:

• Разрушение назначения для доступа к полям в pagesявляется хорошим изменением.
• извлечение parseFoo()функций и т. д., возможно, хорошее изменение.
• Представление функтора ... очень запутанно.

Одна из самых запутанных частей здесь - то, как вы смешиваете функциональное и императивное программирование. С вашим функтором вы на самом деле не преобразуете данные, вы используете его для передачи изменяемого списка через различные функции. Это не очень полезная абстракция, у нас уже есть переменные для этого. То, что, возможно, следовало бы абстрагировать - только синтаксический анализ этого элемента, если он существует - все еще явно присутствует в вашем коде, но теперь мы должны подумать о чем-то другом. Например, несколько неочевидно, что parseFoo(foo)вернет функцию. JavaScript не имеет статической системы типов, чтобы уведомлять вас, допустимо ли это, поэтому такой код действительно подвержен ошибкам без лучшего имени ( makeFooParser(foo)?). Я не вижу никакой пользы в этой путанице.

Что я ожидал увидеть вместо этого:

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

Но это тоже не идеально, потому что с сайта вызовов не ясно, будут ли элементы добавляться в список страниц. Если вместо этого функции синтаксического анализа являются чистыми и возвращают (возможно, пустой) список, который мы можем явно добавить на страницы, это может быть лучше:

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

Дополнительное преимущество: логика о том, что делать, когда элемент пуст, теперь перемещена в отдельные функции анализа. Если это не подходит, вы все равно можете ввести условные выражения. Изменчивость pagesсписка теперь объединена в одну функцию, а не распределяется по нескольким вызовам. Избегание нелокальных мутаций является гораздо большей частью функционального программирования, чем абстракции с такими забавными именами, как Monad.

Так что да, ваш код был слишком умным. Пожалуйста, применяйте свой ум не для написания умного кода, а для того, чтобы найти умные способы избежать необходимости вопиющего умства. Лучшие проекты не выглядят причудливыми, но кажутся очевидными для всех, кто их видит. И хорошие абстракции существуют для того, чтобы упростить программирование, а не добавлять дополнительные слои, которые я сначала должен распутать в своем уме (здесь выясняется, что функтор эквивалентен переменной и может быть эффективно исключен).

Пожалуйста: если сомневаетесь, сохраняйте ваш код простым и глупым (принцип KISS).

Если вы сомневаетесь, это, вероятно, слишком умно! Второй пример вводит случайную сложность с подобными выражениями foo ? parseFoo(foo) : x => x, и в целом код является более сложным, что означает, что ему труднее следовать.

Предполагаемое преимущество, заключающееся в том, что вы можете тестировать блоки по отдельности, может быть достигнуто более простым способом, просто разбившись на отдельные функции. А во втором примере вы объединяете в отдельности итерации, так что вы фактически получаете меньшую изоляцию.

Что бы вы ни думали о функциональном стиле в целом, это, безусловно, пример, когда он делает код более сложным.

Я нахожу немного предупреждающего сигнала в том, что вы ассоциируете простой и понятный код с «начинающими разработчиками». Это опасный менталитет. По моему опыту все наоборот: разработчики-новички склонны к чрезмерно сложному и умному коду, потому что для того, чтобы увидеть самое простое и понятное решение, требуется больше опыта.

Совет против «умного кода» на самом деле не о том, использует ли код передовые концепции, которые новичок может не понять. Скорее речь идет о написании кода, который является более сложным или запутанным, чем необходимо . Это усложняет выполнение кода для всех , как для новичков, так и для экспертов, а также, возможно, и для вас самих через несколько месяцев.

Этот мой ответ приходит с некоторым опозданием, но я все еще хочу вмешаться. То, что вы используете новейшие методики ES6 или самую популярную парадигму программирования, не обязательно означает, что ваш код более правильный, или код этого младшего неправильно. Функциональное программирование (или любой другой метод) следует использовать, когда это действительно необходимо. Если вы попытаетесь найти малейший шанс втиснуть новейшие методы программирования в каждую проблему, у вас всегда будет слишком сложное решение.

Сделайте шаг назад и попытайтесь на секунду озвучить проблему, которую вы пытаетесь решить. По сути, вы просто хотите, чтобы функция addPagesпреобразовывала различные части apiDataв набор пар ключ-значение, а затем добавляла их все PagesList.

И если это все, что нужно, зачем использовать identity functionwith ternary operatorили использовать functorдля анализа ввода? Кроме того, почему вы даже думаете, что это правильный подход, functional programmingкоторый вызывает побочные эффекты (изменяя список)? Зачем все эти вещи, когда все, что вам нужно, это просто:

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

(исполняемая jsfiddle здесь )

Как видите, этот подход все еще используется functional programming, но в меру. Также обратите внимание, что, поскольку все 3 функции преобразования не вызывают никаких побочных эффектов, их очень легко проверить. Код addPagesтакже является тривиальным и непритязательным, что новички или эксперты могут понять только одним взглядом.

Теперь сравните этот код с тем, что вы придумали выше, видите ли вы разницу? Несомненно, functional programmingи синтаксис ES6 является мощным, но если вы неправильно решите проблему с помощью этих методов, вы получите еще более грязный код.

Если вы не спешите с проблемой и применяете правильные методы в нужных местах, вы можете получить код, который будет функциональным по своей природе, но при этом будет очень организованным и поддерживаемым всеми членами команды. Эти характеристики не являются взаимоисключающими.

Второй фрагмент не более тестируем, чем первый. Было бы достаточно просто настроить все необходимые тесты для любого из двух фрагментов.

Реальная разница между этими двумя фрагментами - понятность. Я могу довольно быстро прочитать первый фрагмент и понять, что происходит. Второй фрагмент, не так уж и много. Это гораздо менее интуитивно, а также значительно дольше.

Это облегчает поддержку первого фрагмента, что является ценным качеством кода. Я нахожу очень мало значения во втором фрагменте.

TD; DR

1. Можете ли вы объяснить свой код младшему разработчику за 10 минут или меньше?
2. Через два месяца вы можете понять свой код?

Детальный анализ

Ясность и удобочитаемость

Оригинальный код впечатляюще прост и понятен для любого уровня программиста. Это стиль, знакомый всем .

Удобочитаемость в значительной степени основана на знакомстве, а не на математическом подсчете токенов . ИМО, на данном этапе у вас слишком много ES6 в вашем переписывании. Возможно, через пару лет я изменю эту часть своего ответа. :-) Кстати, мне также нравится ответ @ b0nyb0y как разумный и четкий компромисс.

способность быть свидетелем в суде

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

Предполагая, что PagesList.add () имеет тесты, что и должно быть, это совершенно простой код, и нет никаких очевидных причин, по которым этот раздел нуждается в специальном отдельном тестировании.

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

Опять же, я не вижу непосредственной необходимости какого-либо специального отдельного тестирования этого раздела. Если только у PagesList.add () нет необычных проблем с нулями, дубликатами или другими входными данными.

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

Этот код также очень прост. Предполагая, что customBazParserэто проверено и не возвращает слишком много «специальных» результатов. Опять же, если только не возникнут хитрые ситуации с `PagesList.add (), (что может быть, так как я не знаком с вашим доменом), я не понимаю, почему этот раздел нуждается в специальном тестировании.

В целом, тестирование всей функции должно работать нормально.

Отказ от ответственности : если есть особые причины для проверки всех 8 возможностей этих трех if()утверждений, тогда да, разделите тесты. Или, если PagesList.add()это чувствительно, да, разделите тесты.

Структура: стоит ли разбиваться на три части (как в Галлии)

Здесь у вас есть лучший аргумент. Лично я не думаю, что оригинальный код «слишком длинный» (я не фанат SRP). Но если бы было еще несколько if (apiData.pages.blah)разделов, то SRP поднял бы уродливую голову, и это стоило бы разделить. Особенно, если применяется DRY и функции могут быть использованы в других местах кода.

Мое одно предложение

YMMV. Чтобы сохранить строку кода и некоторую логику, я мог бы объединить if и let в одну строку: например

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

Это не удастся, если apiData.pages.arrayOfBars является Число или Строка, но так же как и исходный код. И мне это понятнее (и излишне употребляемая идиома).