Один день из жизни 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: А пока пошли обсудим новую задачу твою
