Стрелочный ад, или новый круг старой проблемы

Истоки


Когда-то давно JavaScript разработчики очень любили использовать анонимные функции (собственно и сейчас многие их любят использовать), и всё бы ничего, и код становится короче, и не надо придумывать очередное название для функции, но рано или поздно это превращается в “водопад функций” прочитать который вы сможете разве только в хорошем IDE и то немного поломав голову.



Вот вам один пример:


Пример 1:


$http.request('GET', '/api').then(function (data) {
  return data.arr.map(function (item) {
    item.props.filter(function (prop) {
      return !!prop;
    }).map(function (prop) {
      prop.a = prop.a * 2;
      return prop;
    })
  }).filter(function (item) {
    return item.props.some(function (prop) {
      return prop.a > 10;
    });
  });
}, function (error) {
  throw new TypeError(error);
});

Это очень простой пример, в моей практике я встречал “монстров” на 100 — 200, а то и 300+ строк кода, которые приходилось рефакторить, прежде чем понять что за магию они делают.


В своё время переход на “шаблон функционального дизайна”, в котором каждая функция объявляется отдельно и используются только ссылки на них сделал код красивым, аккуратным и самое главное легко читаемым, что очень важно, когда вы работаете с ним не один.


Этот пример, например, преобретает следующий вид:


Пример 2:


$http.request('GET', '/api').then(requestSuccessHandler, requestErrorHandler);

function requestSuccessHandler(result) {
  return result.arr
               .map(transformResultArr)
               .filter(filterResultArr);

  function transformResultArr(item) {
    item.props
        .filter(checkProperty)
        .map(transformProperty)
  }

  function filterResultArr(item) {
    return item.props.some(filterProperty);
  }

  function checkProperty(prop) {
    return !!prop;
  }

  function transformProperty(prop) {
    prop.a = prop.a * 2;
    return prop;
  }

  function filterProperty(prop) {
    return prop.a > 10;
  }
}

function requestErrorHandler(error) {
  throw new TypeError(error);
}

Кода хоть и больше, но зато с таким вариантом проще работать, он легче читается и дебажится.


Но всё хорошее длилось не долго.


Стрелочные функции


Прежде чем перейдём к проблеме, давайте посмотрим что-же такое стрелочный функции и что они нам дали со своим приходом.


Стрелочный функции это короткая запись анонимных функций. В принципе это всё, но на практике это оказывается очень полезно. Например, если вам надо выбрать из массива все элементы имеющие поле id. Раньше это было бы записано так:


Пример 3:


arr.filter(function(item) {
  return !!item.id;
});

Или так:


Пример 4:


arr.filter(filterID);

function filterID(item) {
  return !!item.id;
}

Но со стрелочными функциями эта запись станет неприлично короткой:


Пример 5:


arr.filter(item => !!item.id);

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


Проблема появляется не сразу.


Стрелки во всём


Недавно я перешёл на новый проект, в котором с самого начала активно используется ECMAScript 2015, и как водится везде по максимуму используются новые фичи языка. Самой заметной из них является повсеместное использование стрелочных функций. И всё было бы хорошо, если бы это не были водопады функций на 100+ строк и 4+ уровней вложенности.


Для наглядности вернёмся к первому примеру, в этом проекте он бы выглядел примерно так:


Пример 5:


$http.request('GET', '/api').then(data => {
  return data.arr.map(item => {
    item.props.filter(prop => !!prop)
        .map(prop => {
          prop.a = prop.a * 2;
          return prop;
        })
  }).filter(item => item.props.some(prop => prop.a > 10));
}, error => {
  throw new TypeError(error)
});

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


Но я не говорю, что использовать стрелки плохо, просто их надо использовать в меру.


Как же всё сделать лучше?


На пути создания хороших практик с ECMAScript 2015 мы будем ещё долго спотыкаться и падать, но для начала давайте начнём объединять лучшие старые и новые практики.


Например соединим примеры 2 и 5:


Пример 6:


$http.request('GET', '/api').then(requestSuccessHandler, requestErrorHandler);

function requestSuccessHandler(result) {
  return result.arr
               .map(transformResultArr)
               .filter(filterResultArr);

  function transformResultArr(item) {
    item.props
        .filter(prop => !!prop)
        .map(transformProperty)
  }

  function filterResultArr(item) {
    return item.props.some(prop => prop.a > 10);
  }

  function transformProperty(prop) {
    prop.a = prop.a * 2;
    return prop;
  }
}

function requestErrorHandler(error) {
  throw new TypeError(error);
}

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

Комментарии 50

    +7
    Стрелочный функции это короткая запись анонимных функций. В принципе это всё

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


    А так да, именованные функции, особенно если они длиннее 1-2 строк, проще читать и поддерживать (если они нормально названы, конечно:)).

      +1

      Как насчёт такого варианта?


      class $my_app {
      
          itemsRaw() {
              return $mol_http.resource( '/api' ).json().arr
          }
      
          itemsNormal() {
              var items = this.itemsRaw()
              items.forEach( item => {
                  if( !item.prop ) return
                  item.prop *= 2
              } )
              return items
          }
      
          itemsFiltered() {
              return this.itemsNormal().filter( item => {
                  return item.props.some( prop => prop.a > 10 )
              } )
          }
      
      }
        0

        Точнее так:


        class $my_app {
            itemsRaw() {
                return $mol_http.resource( '/api' ).json().arr
            }
            itemsNormal() {
                var items = this.itemsRaw()
                items.forEach( item => item.prop.forEach( prop => {
                    if( !prop ) return
                    prop.a *= 2
                } ) )
                return items
            }
            itemsFiltered() {
                return this.itemsNormal().filter( item => {
                    return item.props.some( prop => {
                        return prop.a > 10
                    } )
                } )
            }
        }
          0

          Или даже так:


          class $my_app {
          
              itemsFiltered() {
                  return this.itemsNormal().filter( item => this.itemCheck( item ) )
              }
          
              itemCheck( item ) {
                  return item.props.some( prop => prop.a > 10 )
              }
          
              itemsNormal() {
                  return this.itemsRaw().map( item => this.itemNormalize( item ) )
              }
          
              itemNormalize( item ) {
                  item.prop.forEach( prop => {
                      if( !prop ) return
                      prop.a *= 2
                  } )
                  return item
              }
          
              itemsRaw() {
                  return $mol_http.resource( '/api' ).json().arr
              }
          
          }
            +1
            Зачем создавать класс, если здесь нет локальных свойств? Просто чтоб сгруппировать несколько функций? Да и методы должны быть глаголами
              +2

              Например, за этим:


              class $my_app_prefixed extends $my_app {
              
                  itemsNormal() {
                      return [ this.itemPrefix() ].concat( super.itemsNormal() )
                  }
              
                  itemPrefix() {
                      return {
                          prop : [
                              { a : 10 }
                          ]
                      }
                  }
              
              }

              Это процедуры (itemNormalize) и конвертеры (itemCheck) должны быть глаголами. Геттеры же (itemsFiltered, itemsNormalized, itemsRaw) вполне могут именоваться и существительными.

                +2

                $my_app_postfixed =)

        +6
        Мое мнение: стрелочные функции хороши тогда, когда они укладываются в одно выражение (арифметическое или логическое).
        Ну и конечно в случае, если принципиально важно сохранить this.
        Во всем остальном лучше традиционные.
          0
          Ну и конечно в случае, если принципиально важно сохранить this.

          Да и this во многих случаях проще передавать, как параметр.
            0
            Ничто не мешает сделать так, сохранив контекст и выделив имя функции.

            get(...).then(result => processResult(resut))
              0
              Только по моему будет аккуратней если через bind прокинуть this
              get(...).then(processResult.bind(this))
              
                0

                Конкретно в данном случае можно и так, но в общем случае так лучше не делать. Иллюстрация:


                '33333'.split( '' ).map( parseInt ) // [3, NaN, NaN, NaN, 3]
                  0
                  Скажите, а почему так получается?
                    +1

                    Array#map — передает функции обратного вызова 3 аргумента, элемент массива, индекс и сам массив.
                    parseInt(string, radix); — принимает 2 аргумента строку и целое число в диапазоне между 2 и 36, представляющее собой основание системы счисления.


                    parseInt(3, 0) // = 3
                    parseInt(3, 1) // = NaN
                    parseInt(3, 2) // = NaN
                    parseInt(3, 3) // = NaN
                    parseInt(3, 4) // = 3
          • НЛО прилетело и опубликовало эту надпись здесь
              +2
              Да, но в «боевом» коде могут быть снова проблемы с читаемостью. Чаще всего все функции лучше объявлять в конце кода через function, а не как переменную. И почти никогда нет проблемы прокинуть контекст через call, bind или apply.
                0
                Можно, но не в имени дело. Я уже писал.
              +1
              это правда, стрелочки начинают читаться намного хуже после двух строк кода в теле функции
                0
                Точно также в c# лямбды выглядят. Унификация и стандартизация…
                  +2
                  Помниться, про написание программ на C++ Б. Страуструп писал:,"… руководствуйтесь чувством меры, здравым смыслом и подражайте хорошим образцам...". Судя по личному опыту, это трудно. Но, если хочется выразительный язык, то без этого никак. Почему в JS должно быть иначе?
                    0
                    Монструозность JS возрастает очень быстро, ещё лет десять — и он догонит C++. Особенно если кого-нибудь осенит «светлая» идея добавить в стандарт ECMAScript #define/#ifdef.
                    0
                    Просто многие часто забывают, что новая фича синтаксиса зачастует не обеспечивает «хорошего кода» по умолчанию и начинают пихать везде, где нужно и не нужно. Пусть думают сначала, а потом пишут. К тому же обычные анонимные функции никуда не делись и продолжать их испольовать никто не заприщает и в ES6
                      0
                      Проблему частично решает Async/Await, жаль, без babel пока нигде не работает
                        +2
                        А так?
                        $http.request('GET', '/api')
                            .then(data =>
                            {
                                return data.arr
                                    .map(item =>
                                    {
                                        return item.props
                                            .filter(prop => Boolean(prop))
                                            .map(prop => 
                                            {
                                                prop.a = prop.a * 2;
                                                return prop;
                                            });
                                    })
                                    .filter(item => 
                                    {
                                        return item.props.some(prop => prop.a > 10);
                                    });
                            })
                            .catch(e => new TypeError(error));

                        Стало понятнее? На мой взгляд вся проблема тут не в => или function, а в том, что используя множество вложенных функций, такой стиль кода напрочь убивает всю читаемость. Сложно понять, что где начинается, а что где заканчивается. В моём варианте я делаю отступы в цепочках (т.е. уже сами цепочки обособлены, не запутаешься), причём первый же метод в цепочке начинается на 2-й строке (с той же целью — читаемость).


                        А вместо длинных наименований новых методов проще вставлять комментарии. К примеру


                        .filter(item => 
                        {
                            // бла-бла-бла
                            return item.props.some(prop => prop.a > 10);
                         });

                        Кстати говоря, возвращать из .map тот же самый элемент, не совсем канонично :)

                          0
                          Поддерживаю! Отступы играют большую роль в читаемости. Такой пример более читаем, чем вариант с отдельными именнованными функциями.
                            0

                            Про .map хорошо подметили, если не важна мутабельность, тогда уж использовать map по назначению, и тогда всё выглядит ещё более читабельно:


                            Вместо


                            .map(prop => 
                                                {
                                                    prop.a = prop.a * 2;
                                                    return prop;
                                                });

                            С object spread будет


                            .map(prop => ({...prop, a: prop.a * 2}))

                            или если spread режет глаз


                            .map(prop => Object.assign({}, prop, {a: prop.a * 2}))

                            Если всё-таки важна мутабельность и сохранение цепочки, то Object.assign снова же можно заюзать


                            .map(prop => Object.assign(prop, {a: prop.a * 2}))
                              0
                              Прям антипаттерн по-Мартину, ибо его позиция — вместо комментария всегда создавать новую функцию :)
                                +1

                                Тут проблема в том, что метод оторван от контекста. Расположен чёрт знает где и не видно окружения его вызова. Да и именование метода сильно уступает комментарию по выразительности. Если писать вот в таком вот callback-стиле, то попытка раздербанить один средней сложности вложенный кусок кода на полтора десятка методов с длинными непонятными названиями приведёт к полной дезориентации. Как собственно у автора и получилось с его transformResultArr, transformProperty и checkProperty. Мусорные названия однострочных методов вырванных из контекста и расположенных в произвольном порядке. А всего то нужно было отступы правильно задать.


                                В общем тут палка о двух концах. Когда стрелочные анонимки жиреют и становятся сложными — их нужно выносить вовне. Или же когда речь идёт о переиспользуемости кода.

                                  0
                                  Да я не говорю, что я с Мартиным согласен, чай не сразу после прочтения оного пошел в интернет разносить мудрость :) Но смысл в этих словах есть. Писать в callback-стиле вообще печально, а писать так или вот эдак лишь придает определенный оттенок горечи. Поэтому в моем основном языке есть прекрасный async/await (вроде как в JS запилили yield, но я за ним не слежу, не скажу), который весь этот коллбек-колбасу превращает в абсолютно линейный код. И даже оступов сверх обычного никаких не нужно.
                                    0

                                    async/await касается только асинхронного кода. Синхронные цепочки трансформации данных никуда не деваются. Хотя наверное "callback-ый ад" — это не про них :) Но приведённый в примерах код как раз о них.

                                      0

                                      Забыл сказать, мне в scala понравилось то, как удобно там организованы простые случаи callback-ов:


                                      collection
                                        .map(el => el.export())
                                        .filter(el => el.age > 10)
                                      // =>
                                      collection.map(_.export).filter(_.age > 10)

                                      Плюс стандартная библиотека умеет большую часть насущных вещей из lodash.js.

                                        0
                                        прекрасный async/await

                                        В том, что у вас весь код усеян асинками да авайтами нет ничего прекрасного. В ноде можно писать безо всей этой ерунды:


                                        var Future = require( 'fibers/future' )
                                        var Fetch = require( 'fetch-promise' )
                                        
                                        class Transport {
                                        
                                            fetch( uri ) {
                                                return Future.fromPromise( Fetch( uri ) ).wait()
                                            }
                                        
                                            fetchJSON( uri ) {
                                                return JSON.parse( this.fetch( uri ).buf )
                                            }
                                        
                                            fetchTable( uri ) {
                                                return this.fetchJSON( uri ).data.children.map( ({ data })=> data )
                                            }
                                        
                                        }
                                          0

                                          Не желаете написать обзорную статью про fibers в nodeJS? :) Было бы интересно узнать про все подводные камни и сравнение с теми же async-promise-ми. Такие вопросы как производительность, принцип работы волокон (признаться, я вот так его и не понял), обработка ошибок, сегфолты (когда баловался — часто их наблюдал). В частности интересно, почему такой подход в nodeJS есть давно, но всё остаётся уделом одиночек?

                                            +3

                                            Давно хочу написать/записать, но руки не доходят.


                                            Async функция — это фактически обёртка над генератором. Генератор — это фактически конечный автомат. А конечный автомат — это фактически switch-case с выбором ветки кода в зависимости от значения счётчика.


                                            Волокно — это указатель на стек. В любой момент вы можете "заморозить" текущее волокно, передав управление другому (изменив указатель на другой стек), а по какому-либо событию "разморозить" и продолжить исполнение. Так как изменение указателя — тривиальная операция, волокна ещё называют "легковесными потоками". То есть они обеспечивают многопоточность даже в однопоточной среде.


                                            Ошибки обрабатываются стандартно — через try-catch. При этом рекомендуется использовать Future (это аналог промисов, но вместо then и catch, используется wait), которые исправляют стектрейсы.


                                            С сегфолтами не сталкивался, но расширение это довольно зрелое. А мало кем используется потому, что нет хайпа и в браузере не заведётся.


                                            Минусы волокон:


                                            1. Это не стандарт.
                                            2. Это бинарное расширение.
                                            3. Работает только в NodeJS.
                                            4. Отладчик (во всяком случае в вебшторме) не может получить значения переменных после пробуждения волокна.
                                            5. Большее потребление памяти в сравнении с генераторами (необходимо хранить стек волокна).

                                            Плюсы волокон:


                                            1. Это расширение платформы, а не языка.
                                            2. Асинхронность инкапсулируется в одной функции и не распространяется как вирус по всему приложению.
                                            3. Больше скорость в сравнении с генераторами. Накладные расходы только на создание и переключение волокна. Генератор даёт пенальти на каждый свой вызов.

                                            Ну и для полноты картины: сравнение кодов с разной реализацией асинхронности.

                                              +1

                                              Я там по ссылке добавил замеры времени.


                                              1. Самой быстрой, неожиданно, оказалась асинхронная версия на атомах. Видимо сказывается минимум замыканий.
                                              2. Незначительно отстаёт синхронная версия.
                                              3. С почти двухкратным отставанием идут асинхронные версии на колбэках и файберах.
                                              4. Чуть погодя — асинхронные версии на промисах и генераторах.
                                              5. И ещё в 3 раза медленней — то, что сгенерировал Babel из async/await.
                                                +1

                                                Ну и совсем для полноты картины добавил примеры стек трейсов.


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

                                    +1
                                    Комментарии всегда намного хуже отдельных методов с говорящими названиями.
                                      +1
                                      Комментарии всегда намного хуже отдельных методов с говорящими названиями.

                                      Это чем же?


                                      • вы дублируете говорящее_название_метода дважды (вызов + определение метода)
                                      • вы ограничены правилами именования методов, да ещё и codeStyle-ом
                                      • имя метода максимум 1 строка
                                      • тело метода (в данном контексте одно-двустрочник) располагается отдельно (и возможно далеко) от места использования
                                      • для того чтобы имя метода стало говорящим в отрыве от контекста может потребоваться намного больше слов, чем комментарию по месту
                                      • в зависимости от области видимости может возникнуть именная коллизия

                                      За примерами ходить не нужно — в статье их навалом. Самый глупый метод — checkProperty. Вообще ни о чём не говорящее название. А при использовании lodash всего то потребовалось бы .compact(). По сути если стараешься писать в около-функциональном стиле, то выносить одно-двух строчные очевидные действия в отдельные методы — сильно ухудшать кодовую базу. Всё сразу становится сложным и непонятным.

                                        +1
                                        Одно-двух строчные очевидные действия и в комментариях не нуждаются.
                                          0
                                          Тут я соглашусь с тем, что вам ответил PsyHaSTe — к очевидным действиям и комментарии не нужны. Они только замусоривают код. IDE показывает их серым, и глаз их игнорирует.

                                          Если же есть несколько строк кода, выполняющих какую-то одну цельную _функцию_ с неким смыслом — они так и должны быть выделены в функцию. Где будет ее тело, не особо важно, т.к. в IDE есть переход к определению и обратно.

                                          Отдельно остановлюсь тут — «вы дублируете говорящее_название_метода дважды (вызов + определение метода)».
                                          Извините, но это не дублирование. Дублирование — это фразы, имеющие одинаковый смысл. Вызов и определение метода никаким дублированием, конечно, не является. А вот когда блоки кода не выносятся в функцию — это провоцирует и дублирование, и что еще хуже — написание заново кода с тем же функционалом в другом месте.
                                            0
                                            Где будет ее тело, не особо важно, т.к. в IDE есть переход к определению и обратно.

                                            А толку то от этого? Вы теряете контекст. Для одно-дву-строчных методов это критично. А именно о них мы и говорим.

                                      +2
                                      Ну правильно давайте все фигачит именными функциями:) Может все таки не зря сахар для классов сделали?
                                        0
                                        Судя по рекомендациям от инженеров Google подход с анонимными функциями еще намного медленнее, чем объявление их отдельно, т.к. при каждом запросе создается новая анонимная новая функция, если правильно все понял.
                                          0
                                          ссылка не прикрепилась https://developers.google.com/speed/articles/optimizing-javascript
                                            0
                                            Так и есть. А потом еще и удаляется, вызывая фризы GC. Ну и слабо оптимизируются, так как мало раз выполняются.
                                            В этом плане обычные замыкания от «стрелочных» никак не отличаются. Но боятся этого надо когда такое создание/уничтожение имеет массовый характер.
                                            И можно было бы сделать (и имело бы смысл) код как в примере №2 — он был бы самым быстрым и про однократной работе коде, и, особенно, при многократной.
                                            +1
                                            Сначала хочется отметить, что раз речь идет о функциональном подходе, то стоит в примерах стремиться и к чистоте функций. И именованные функции — это конечно, замечательно, но здесь также надо знать меру, ровно как и со стрелочными функциями.

                                            К слову, в примере со стрелочными функциями data.arr заполнится undefined.
                                            А приведенный пример остается достаточно понятным и читабельным как-нибудь так (дальнейшая декомпозиция зависит уже от контекста и условий задачи, тестирования и многих факторов):

                                            const requestErrorHandler =…

                                            const requestSuccessHandler = ({ arr }) => arr
                                            .map(({ props }) => props
                                            .filter(prop => !!prop)
                                            .map(prop => ({ ...prop, a: prop.a * 2 })))
                                            .filter(({ props }) => props
                                            .some(({ a }) => a > 10));

                                            $http
                                            .request('GET', '/api')
                                            .then(requestSuccessHandler, requestErrorHandler);
                                              0
                                              Хабр порезал разметку, код должен был выглядеть как-то так: https://jsfiddle.net/nsza9z5u/

                                              const requestErrorHandler = …
                                              
                                              const requestSuccessHandler = ({ arr }) => arr
                                                .map(({ props }) => props
                                                  .filter(prop => !!prop)
                                                  .map(prop => ({ ...prop, a: prop.a * 2 })))
                                                .filter(({ props }) => props
                                                  .some(({ a }) => a > 10));
                                              
                                              $http
                                                .request('GET', '/api')
                                                .then(requestSuccessHandler, requestErrorHandler);
                                              
                                              0

                                              Мне кажется, что до тех пор пока вы используете только методы массива все нормально: array.map().map().filter().map().etc(). Сложности начинаются когда это все в одном месте с промисами или чем-то похожим, т.е. в вашем примере достаточно вынести код трансформирующий результат в отдельный метод и всем все будет ясно, можно дополнительно написать обертку над request которая будет принимать опциональную функцию трансформер.

                                                +1
                                                Посмотрите на исходный код http://horizon.io/, вот там стрелочный ад.
                                                  0

                                                  Именно о таком я и писал :)


                                                  Просто в статью слишком большие примеры плохо пихать, так что я ограничился парой небольших абстрактных примеров.


                                                  А horizon.io явно местами переборщили. К примеру такая запись


                                                  const read_config_from_env = () => {}

                                                  вместо обычного


                                                  function read_config_from_env () {}

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


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

                                                Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                                Самое читаемое