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

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

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

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

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

Продолжение (по результатам работы над комментариями)

...

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

TL: Привет. Да, конечно, давай, рассказывай

Dev:

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

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

• Ну и код еще чуть более упростится, если мы немножко больше данных на выход отдадим - я говорю про поле Group. Думаю, ничего страшного при этом не случится.

TL: Звучит все очень здраво. Show me the code ))

Dev: Держи

const _ = (childIds, parents) => {
  const set = new Set(childIds);
  const data = parents
    .flatMap(e => e.Children.map(x => ({ ...x, ParentName: e.Name, Group: e.Group?.Name })))
    .filter(e => set.has(e.Id));
  return Object.groupBy(data, e => e.Group || "NoGroup");
};

export default _;

TL: Да, отлично! Мы полностью избавились от сторонних зависимостей. Хорошо, что ты нашел Object.groupBy, иначе пришлось опять же через reduce делать, что выглядит немножко не естественно для такой операции как группировка данных по полю. Я смотрю, ты еще и pipe выкинул?

Dev: Да, мне показалось, что и так код прекрасно читается.

TL: Соглашусь. Оставляем этот вариант. Только тесты немножко придется поправить, у нас чуть поменялась структура данных на выходе.

Dev: Да, уже делаю.