A Day in the Life of a JavaScript Developer and His Tech Lead

Hello, friends!

As part of my job, I spend a lot of time reviewing my team's code. Code reviews have become such a part of my life that I decided to put an excerpt from one of them on paper.

In this short note, we offer you a dialogue between a developer and his lead on code review. Let's see how they come to the final decision, what stages the code goes through during refactoring, how the lead and his protégé reason, and what a beautiful final solution they come up with.

If you're interested, let's go.

TL: Hi, I'm finishing up my review of your task. Overall, everything looks pretty good, we're almost there. The only thing that raises some questions for me is the function processLet's take a closer look at it.

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;

It looks a bit confusing to me. As far as I understand, we have 2 collections with data at the input and some pipeline for their processing is launched inside the function. To be honest, this is the only thing I understand so far )). Although, wait, now I'll look at the tests that you wrote, I think it will become a little clearer

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" }
      ]
    });
  });
});

Yeah, I think I'm starting to see what this means.

I see this algorithm like this:

  • At the input we have two collections – a collection of childIds identifiers and a collection of parents objects. Each parent object has a nested Children collection.

  • Next, we essentially unnest the nested Children collection and for each child element we look for a match for its Id in the childIds collection.

  • If we find a match, we group the data by the Group.Name field of Parent

  • Yes, well, and at the output we don’t need all the fields, but only some subset of them

    Is that all right?

Dev: Yes, it seems you got it right. Let me describe the algorithm again in the form of a diagram, I think it will be even clearer.

TL: Yeah, go ahead, it'll make my life easier.

Dev: This is what I got

Data processing algorithm

Data processing algorithm

TL: Aha, it all became clear now. Look, you even described your entire algorithm in terms of some data transformation operations – nest, join, groupBy, select. But looking at your code, I have a hard time understanding where all these operations take place. Let's try to rewrite your algorithm so that it looks like some kind of data pipeline.

Dev: So we somehow need to implement all these blocks – unnest, join, groupBy and select?

TL: Yes, that's right. It's not difficult, it seems. We use the library in the project lodashwhich significantly simplifies the solution of such problems when working with collections and objects.

Dev: Yes, I'll try to do it now

WITH nest everything turns out pretty simple

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

groupBy I also have no questions, the ready function already exists

selectI think it is also not difficult to implement using mapValuesFor example.

And here it is join I can't find anything yet.

TL: Please spend half an hour of your time, I think there should already be a ready-made function that does the operation join.

…

Dev: No, I still haven't found anything.

TL: It's strange, it seemed to me that it should be in lodash be it. Maybe I got it mixed up with the library Ramda

Oh well, it's no big deal, joinit is not difficult to implement it yourself. In any case, sooner or later, you will have to expand lodash with our helper functions, let's start doing that now. Let's put this function in the layer Shared of our application, since it should be very generalized

Dev: Okay, I'll do it now.

…

Dev: Something like that

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: Hmm, you decided to implement it hash join, as far as I can see. It looks right. Maybe in nested loops or merge join would be a bit easier. Don't forget to cover it with tests. Perhaps in the future it will be useful to expand our developments – to have not only inner join, but also left join and right join. But at this stage, I think this will be quite enough.

Dev: That is, it turns out that we already have everything we need to rewrite our algorithm in a slightly more functional way.

TL: Yes, it turns out that way. Forward )

Dev: I'll do it within an hour.

…

Dev: I did everything

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: Great. This is a huge step up from the original version. The functional approach has worked well here.

Dev: The task is ready. Shall we send it for testing?

TL: Take your time. Do you have any other ideas on what could be improved? Pay attention to the variables in which you store the result of each processing step.

Dev: Don't you like their names?

TL: No, I don't really like the fact that they exist. They are used once only to pass data to the next step in the processing pipeline. I wonder if it's possible to get rid of them altogether?

Dev: Give me some time to think, nothing good comes to mind right now.

Well, of course, except for this option

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: No, I don’t like this option, try to guess why?

Dev: Maybe because it turns out that the algorithm is essentially written backwards and must be read from right to left?

TL: Exactly. This makes it very difficult to understand.

Dev: I understand, I'll look for other options

TL: By the way, you once said that you were familiar with F#

Dev: Yes, I am studying it.

TL: How would you solve the problem you voiced with variables if you were writing in F#?

Dev: Exactly, how come I didn't guess it right away. Are you talking about pipes?

TL: Yes, about them.

Dev: I didn't know that in JS they are. Now I'll try to sketch out an option

…

Dev: Look

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

Just one nuance. As far as I read, today this proposal for the operator pipe is in stage 2 of review. This means that even if everything goes according to plan, the new operator will be standardized in about two years. Also, the syntax and capabilities may undergo some changes. Do you think it's worth using?

TL: You're probably right, let's not. But the code looks pretty cool, in my opinion. But it's not hard to simulate something like that yourself…

Dev: Could you help me with this, I don't know yet how it should look

TL: I have in my head approximately the following code structure that might result in the output

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: Looks really cool. So you want me to implement the take function?

TL: Yes, take, pipe And result.

Dev: I'm withdrawing into myself)

…

Dev: It worked

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: This is exactly what I wanted.

Dev: Great. Everything is ready. What task should I take next?

TL: You're in a hurry again. Let's do another exercise?

Dev: Let's

TL: I think we can almost completely, if not completely, get rid of the use of in our code lodasheverything can be solved using native functions. Perhaps it will be a little more optimal. Just look at the browser support for the functions you will replace lodash

Dev: Yes, I see, good idea.

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: So, it seems to have become a little less clear than it was before. As I see, the code was rewritten to use native functions flatmap And map. Let's stop here, in all other cases we will use lodash. This will add readability to our code while maintaining acceptable performance.

Dev: I will do it

…

Dev: Here:

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: Excellent. I suggest we stop here.

Dev: Phew)

TL: Let's compare the original solution and the final one?

Dev: It's a bit scary…

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: It would seem like a small function, but how deep can you dig:

  • Functional approach, when the algorithm acquires the features of a pipeline

  • Borrowing ideas from other programming languages ​​- remember the pipe operator and our solution that mimics it?

  • Getting rid of third-party dependencies in favor of native implementation

  • It is also useful to draw various diagrams before you start writing code. Often, you discover amazing secrets 🙂

Dev: Yes, it turned out to be a pretty good result, in my opinion. I think Martin Fowler talked about this kind of refactoring, calling it Replace Loop with Pipeline

TL: I'll refresh my memory of his works 🙂

TL: In the meantime, let's go discuss your new task.

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *