Один день из жизни JavaScript разработчика и его техлида

Привет, друзья!

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

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

Если вам это интересно, поехали.

TL: Привет, заканчиваю ревью твоей задачи. В целом, все выглядит довольно неплохо, мы почти у цели. Единственное, что вызывает у меня некоторые вопросы, это функция process. Давай на нее посмотрим чуть более пристально.

import isEmpty from "lodash/isEmpty"; import forEach from "lodash/forEach"; import find from "lodash/find";  const process = (childIds, parents) => {   const result = {};   if (!isEmpty(childIds) && !isEmpty(parents)) {     const noGroup = [];     forEach(childIds, childId => {       forEach(parents, parent => {         const childInfo = find(parent.Children, ["Id", childId]);         if (!isEmpty(childInfo)) {           const childData = { ...childInfo, ParentName: parent.Name };           if (parent.Group) {             if (result[parent.Group.Name]) result[parent.Group.Name].push(childData);             else result[parent.Group.Name] = [childData];           } else {             noGroup.push(childData);           }         }       });     });     if (!isEmpty(noGroup)) result.NoGroup = noGroup;   }   return result; };  export default process;

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

import process from "./algorithm";  describe("algorithm", () => {   it("Empty ids", () => {     const result = process([], [{ Name: "Parent", Children: [{ Id: 1, Name: "Child" }] }]);     expect(result).toEqual({});   });    it("Empty parents", () => {     const result = process([1], []);     expect(result).toEqual({});   });    it("Empty collections", () => {     const result = process([], []);     expect(result).toEqual({});   });    it("Empty group", () => {     const result = process([1], [{ Name: "Parent", Children: [{ Id: 1, Name: "Child" }] }]);     expect(result).toEqual({       NoGroup: [{ Id: 1, Name: "Child", ParentName: "Parent" }]     });   });    it("Non empty group", () => {     const result = process(       [1],       [         {           Name: "Parent",           Group: { Name: "Group" },           Children: [{ Id: 1, Name: "Child" }]         }       ]     );     expect(result).toEqual({       Group: [{ Id: 1, Name: "Child", ParentName: "Parent" }]     });   });    it("Several parents", () => {     const result = process(       [1],       [         {           Name: "Parent1",           Group: { Name: "Group" },           Children: [{ Id: 1, Name: "Child1" }]         },          {           Name: "Parent2",           Group: { Name: "Group" },           Children: [{ Id: 2, Name: "Child2" }]         }       ]     );     expect(result).toEqual({       Group: [{ Id: 1, Name: "Child1", ParentName: "Parent1" }]     });   });    it("Complex case", () => {     const result = process(       [1, 2],       [         {           Name: "Parent1",           Group: { Name: "Group" },           Children: [{ Id: 1, Name: "Child1" }]         },          {           Name: "Parent2",           Group: { Name: "Group" },           Children: [{ Id: 2, Name: "Child2" }]         }       ]     );     expect(result).toEqual({       Group: [         { Id: 1, Name: "Child1", ParentName: "Parent1" },         { Id: 2, Name: "Child2", ParentName: "Parent2" }       ]     });   }); });

Ага, я, кажется, начинаю понимать, в чем тут смысл

Вижу данный алгоритм так:

• На входе у нас две коллекции — коллекция идентификаторов childIds и коллекция объектов parents. У каждого объекта parent есть вложенная коллекция Children.

• Далее, по сути, мы делаем unnest вложенной коллекции Children и для каждого дочернего элемента ищем соответствие его Id в коллекции childIds

• Если находим соответствие, то группируем данные по полю Group.Name у Parent

• Да, ну и на выходе нам нужны не все поля, а только некоторое их подмножество

  Все так?

Dev: Да, кажется, ты все правильно понял. Давай я еще раз опишу алгоритм в виде диаграммы, так, думаю, будет еще понятнее

TL: Да, давай, это упростит мне жизнь

Dev: Вот, что у меня получилось

TL: Ага, стало все понятно теперь. Смотри, ты уже даже описал весь свой алгоритм в терминах неких операций трансформации над данными — unnest, join, groupBy, select. Но, глядя на твой код, я с трудом понимаю, где же происходят все эти операции. Давай попробуем переписать твой алгоритм таким образом, чтобы он и в коде выглядел как некий data pipeline.

Dev: То есть нам как-то нужно реализовать все эти блоки — unnest, join, groupBy и select?

TL: Да, все так. Это не сложно, кажется. Мы в проекте используем библиотеку lodash, значительно упрощающую решение подобных задач по работе с коллекциями и объектами.

Dev: Да, сейчас попробую сделать

С unnest все довольно просто выходит

flatMap(e => e.Children.map(a => ({ Parent: e, Child: a })))

groupBy так же у меня не вызывает вопросов, готовая функция уже есть

select, думаю, тоже не сложно реализовать с использованием mapValues, например.

А вот join я что-то не нахожу пока.

TL: Потрать, пожалуйста, полчаса времени, думаю, должна уже быть готовая функция, которая делает операцию join.

…

Dev: Нет, так и не нашел я ничего

TL: Странно, мне казалось, должно в lodash быть это. Может быть, я перепутал с библиотекой Ramda

Ну да ладно, не беда, join, не сложно реализовать самим. В любом случае, рано или поздно, придется расширять lodash своими вспомогательными функциями, давай начнем это делать сейчас. Положим эту функцию в слой Shared нашего приложения, так как она очень обобщенной должна получиться

Dev: Хорошо, сейчас сделаю

…

Dev: Как-то так

import has from "lodash/has"; import map from "lodash/map"; import groupBy from "lodash/groupBy"; import reduceRight from "lodash/reduceRight";  const innerJoin = (a, aAccessor, b, bAccessor, merger) => {   const aHash = groupBy(a, aAccessor);   return reduceRight(     b,     (agg, bData) => {       const bDataHash = bAccessor(bData);       if (has(aHash, bDataHash)) {         return map(aHash[bDataHash], e => merger(e, bData)).concat(agg);       }       return agg;     },     []   ); };  export default innerJoin;

TL: Хм, ты решил реализовать hash join, на сколько я вижу. Выглядит правильно. Возможно, через nested loops или merge join было бы чуть проще. Не забудь тестами только ее покрыть. Возможно, в будущем будет полезно расширить наши наработки — иметь не только inner join, но и left join и right join. Но на данном этапе, думаю, этого будет вполне достаточно.

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

TL: Да, получается, что так. Вперед )

Dev: В течение часа сделаю.

…

Dev: Все сделал

import groupBy from "lodash/groupBy"; import flatMap from "lodash/flatMap"; import mapValues from "lodash/mapValues"; import map from "lodash/map"; import innerJoin from "./innerJoin";  const _ = (childIds, parents) => {   const unnested = flatMap(parents, e => map(e.Children, a => ({ Parent: e, Child: a })));   const joined = innerJoin(unnested, x => x.Child.Id, childIds, x => x, x => x);   const grouped = groupBy(joined, x => x.Parent.Group?.Name || "NoGroup");   const selected = mapValues(grouped, o =>     map(o, x => ({ ...x.Child, ParentName: x.Parent.Name }))   );   return selected; };  export default _; 

TL: Супер. Это огромный шаг вперед по сравнению с первоначальной версией. Здесь хорошо зашел функциональный подход.

Dev: Задача готова. Отправляем в тестирование?

TL: Не торопись. Нет ли у тебя еще идей, что можно улучшить. Обрати внимание на переменные, в которых ты хранишь результат каждого шага обработки.

Dev: Тебе не нравятся их названия?

TL: Нет, мне не очень нравится сам факт их наличия. Они используются один раз лишь для того, чтобы передать данные на следующий шаг пайплайна обработки. Я думаю, можно ли от них совсем избавиться?

Dev: Дай мне время подумать, сходу пока не приходит ничего хорошего в голову.

Ну, конечно, кроме такого варианта

import groupBy from "lodash/groupBy"; import flatMap from "lodash/flatMap"; import mapValues from "lodash/mapValues"; import map from "lodash/map"; import innerJoin from "./innerJoin";  const _ = (childIds, parents) => {   return mapValues(     groupBy(       innerJoin(         flatMap(parents, e => map(e.Children, a => ({ Parent: e, Child: a }))),         x => x.Child.Id,         childIds,         x => x,         x => x       ),       x => x.Parent.Group?.Name || "NoGroup"     ),     o => map(o, x => ({ ...x.Child, ParentName: x.Parent.Name }))   ); };  export default _; 

TL: Нет, такой вариант мне не нравится, попробуй догадаться почему?

Dev: Может быть, потому, что получается, что алгоритм, по сути, написан задом на перед и читать его надо справа налево?

TL: В точку. Это очень усложняет понимание.

Dev: Я понял, поищу другие варианты

TL: Кстати, ты как-то говорил, что знаком с F#

Dev: Да, изучаю его

TL: Как бы ты решил озвученную проблему с переменными, если бы писал на F#?

Dev: Точно, как я сразу не догадался. Ты о пайпах?

TL: Да, о них.

Dev: Не знал, что в JS они есть. Сейчас попробую набросать вариант

…

Dev: Смотри

import groupBy from "lodash/groupBy"; import flatMap from "lodash/flatMap"; import mapValues from "lodash/mapValues"; import map from "lodash/map"; import innerJoin from "./innerJoin";  const _ = (childIds, parents) => {   return parents     |> flatMap(%, e => map(e.Children, a => ({ Parent: e, Child: a })))     |> innerJoin(%, x => x.Child.Id, childIds, x => x, x => x)     |> groupBy(%, x => x.Parent.Group?.Name || "NoGroup")     |> mapValues(%, o => map(o, x => ({ ...x.Child, ParentName: x.Parent.Name }))); };  export default _; 

Только один нюанс. На сколько я прочитал, на сегодняшний день данное предложение по оператору pipe находится на 2 стадии рассмотрения. Это означает, что даже если все пойдет по плану, новый оператор будет стандартизирован года через два. Кроме того, синтаксис и возможности могут претерпеть некоторые изменения. Думаешь, стоит его использовать?

TL: Наверное, ты прав, давай не будем. Но код выглядит очень классно, на мой взгляд. Но ведь не сложно сымитировать что-то подобное самостоятельно…

Dev: Не мог бы ты мне с этим помочь, что-то пока не знаю, как это должно выглядеть

TL: У меня в голове примерно такая структура кода, которая может получиться на выходе

import groupBy from "lodash/groupBy"; import flatMap from "lodash/flatMap"; import mapValues from "lodash/mapValues"; import map from "lodash/map"; import innerJoin from "./innerJoin"; import take from "./pipe";  const _ = (childIds, parents) =>   take(parents)     .pipe($ => flatMap($, e => map(e.Children, a => ({ Parent: e, Child: a }))))     .pipe($ => innerJoin($, x => x.Child.Id, childIds, x => x, x => x))     .pipe($ => groupBy($, x => x.Parent.Group?.Name || "NoGroup"))     .pipe($ => mapValues($, o => map(o, x => ({ ...x.Child, ParentName: x.Parent.Name }))))     .result();  export default _;

Dev: Выглядит очень круто. То есть ты хочешь, чтобы я реализовал функцию take?

TL: Да, take, pipe и result.

Dev: Ухожу в себя )

…

Dev: Получилось

class Pipe {   constructor(value) {     this.firstArg = value;   }    pipe(fn, ...args) {     const result = fn(...[this.firstArg].concat(args));     return new Pipe(result);   }    result() {     return this.firstArg;   } }  const take = value => new Pipe(value);  export default take;

TL: Это именно то, что я хотел.

Dev: Супер. Все готово. Какую мне следующую задачу взять?

TL: Опять ты торопишься. Давай еще одно упражнение проведем?

Dev: Давай

TL: Мне кажется, мы можем почти полностью, а может быть и полностью, избавиться в нашем коде от использования lodash, все можно решить с использованием нативных функций. Возможно, это будет чуть более оптимально. Только смотри на поддержу браузерами функций, на которые будешь заменять lodash

Dev: Да, понял, хорошая идея.

import groupBy from "lodash/groupBy"; import innerJoin from "./innerJoin"; import take from "./pipe";  const _ = (childIds, parents) =>   take(parents)     .pipe($ => $.flatMap(e => e.Children.map(a => ({ Parent: e, Child: a }))))     .pipe($ => innerJoin($, x => x.Child.Id, childIds, x => x, x => x))     .pipe($ => groupBy($, x => x.Parent.Group?.Name || "NoGroup"))     .pipe($ =>       Object.keys($).reduce(         (acc, key) => ({           ...acc,           [key]: $[key].map(x => ({ ...x.Child, ParentName: x.Parent.Name }))         }),         {}       )     )     .result();  export default _; 

TL: Так, кажется, стало чуть менее понятным, чем было раньше. Как я вижу, код удалось переписать на использование нативных функций flatMap и map. Давай на этом остановимся, во всех остальных случаях будем использовать lodash. Это добавит читаемости нашему коду, сохранив приемлемую производительность.

Dev: Сделаю

…

Dev: Вот:

import groupBy from "lodash/groupBy"; import mapValues from "lodash/mapValues"; import innerJoin from "./innerJoin"; import take from "./pipe";  const _ = (childIds, parents) =>   take(parents)     .pipe($ => $.flatMap(e => e.Children.map(a => ({ Parent: e, Child: a }))))     .pipe($ => innerJoin($, x => x.Child.Id, childIds, x => x, x => x))     .pipe($ => groupBy($, x => x.Parent.Group?.Name || "NoGroup"))     .pipe($ => mapValues($, o => o.map(x => ({ ...x.Child, ParentName: x.Parent.Name }))))     .result();  export default _;

TL: Отлично. Предлагаю на этом остановиться.

Dev: Фуф )

TL: Сравним изначальное решение и финальное?

Dev: Как-то страшновато…

import isEmpty from "lodash/isEmpty"; import forEach from "lodash/forEach"; import find from "lodash/find";  const process = (childIds, parents) => {   const result = {};   if (!isEmpty(childIds) && !isEmpty(parents)) {     const noGroup = [];     forEach(childIds, childId => {       forEach(parents, parent => {         const childInfo = find(parent.Children, ["Id", childId]);         if (!isEmpty(childInfo)) {           const childData = { ...childInfo, ParentName: parent.Name };           if (parent.Group) {             if (result[parent.Group.Name]) result[parent.Group.Name].push(childData);             else result[parent.Group.Name] = [childData];           } else {             noGroup.push(childData);           }         }       });     });     if (!isEmpty(noGroup)) result.NoGroup = noGroup;   }   return result; };  export default process;

import groupBy from "lodash/groupBy"; import mapValues from "lodash/mapValues"; import innerJoin from "./innerJoin"; import take from "./pipe";  const _ = (childIds, parents) =>   take(parents)     .pipe($ => $.flatMap(e => e.Children.map(a => ({ Parent: e, Child: a }))))     .pipe($ => innerJoin($, x => x.Child.Id, childIds, x => x, x => x))     .pipe($ => groupBy($, x => x.Parent.Group?.Name || "NoGroup"))     .pipe($ => mapValues($, o => o.map(x => ({ ...x.Child, ParentName: x.Parent.Name }))))     .result();  export default _;

TL: Казалось бы, маленькая функция, а как глубоко можно копнуть:

• Функциональный подход, когда алгоритм приобретает черты пайплайна

• Заимствование идей из других языков программирования — помнишь про оператор pipe и наше решение, имитирующее его?

• Избавление от сторонних зависимостей в пользу нативной реализации

• Так же полезно порисовать разного рода диаграммы, прежде чем начинать писать код. Зачастую, открываешь для себя удивительные тайны 🙂

Dev: Да, неплохой результат в итоге получился, на мой взгляд. Кажется, Мартин Фаулер говорил о подобного рода рефакторинге, называя его Replace Loop with Pipeline

TL: Освежу в памяти его труды 🙂

TL: А пока пошли обсудим новую задачу твою