2
\$\begingroup\$

I have a function which returns an array of datasets for a chart written with vue-chartjs. The getter getChartData returns an array with three objects with nested objects. I see that my function has a so called 'callback hell' and I want to refactor this.

getDatasets() {
  let datasetLabels = [];
  const obj = this.getChartData;
  Object.entries(obj).forEach((entry) => {
    datasetLabels = [...datasetLabels, ...Object.keys(entry[1])];
  });
  let datasets = [];
  [...new Set(datasetLabels)].forEach((item, index, arr) => {
    datasets.push({ label: item, backgroundColor: this.getColor(arr, index), data: [] });
  });
  Object.entries(obj).forEach((el) => {
    const datasetData = Object.entries(el[1]);
    datasetData.forEach(
      (item) => {
        datasets = datasets.map((e) => {
          if (e.label === item[0]) {
            e.data.push(item[1].median);
          }
          return e;
        });
      });
  });
  return datasets;
},
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight May 22 '18 at 8:23
  • 2
    \$\begingroup\$ No need to wait until next time - you can still edit to improve your question (but we don't allow you to change the actual code once you've received an answer!). \$\endgroup\$ – Toby Speight May 22 '18 at 9:08
  • \$\begingroup\$ Already done. :) \$\endgroup\$ – Olga B May 22 '18 at 9:35
  • 1
    \$\begingroup\$ Please take note of the link that Toby provided, it states: "Use a title that is catchy, and describes the problem your code solvesUse a title that is catchy, and describes the problem your code solves" please describe what the code does in your title, not an issue you wish to resolve. \$\endgroup\$ – Malachi May 22 '18 at 13:44
5
\$\begingroup\$

False alarm! No callback hell here.

While this code can certainly be refactored a bit, I fail to see callback hell anywhere around. Callback hell is normally a term applied to the code that is executed asynchronously and with multiple nested callback functions. The nested structure is what causes a few issues:

  • cognitive complexity (which leads to bugs and maintenance complication);
  • refactoring difficulty;
  • more importantly, hard time expressing conditional ("branching") or cyclic ("loops") logic.

A related question on StackOverflow might be helpful (just don't focus on RxJs part of the answer there).

Your code does actively use arrow functions, but it is not equivalent to callback hell. The number one reason is because everything in getDatasets() is executed synchronously.

Refactoring

Nitpicking

  • Small details like line breaks are powerful. Splitting the function into paragraphs with empty line separators helps identifying logical steps you're making in code. It also can work help in extracting sub-functions later, if/when the function becomes too long.
  • Naming things: Spelling things out is always better than using names like el, e, arr. Abbreviated variable names like that suggest that these entities have no reason to exist or are unimportant. Even item is not a good name. Since it denotes a label, call it a label. I'm sure you got the idea.
  • Naming continued: Name your variables consistently. You iterate two times over the same object: Object.entries(obj).forEach(...) in one case you call the current entry as entry, while in the other – just el. That is confusing.
  • I like how you lock down the chart data in a locally scoped obj variable. Renaming it to chartData would make it more readable, though.
  • You could also avoid let in a few places and have const instead. For example, reduce(...) or map(...) are in many cases a better alternative to for (...) or .forEach(...).
  • Also, be careful when you deal with array deconstruction. While it's readable, it's also very inefficient because it creates a new array by iterating all the elements of the source arrays. It's only fine to use it for pieces of code that are not invoked frequently and/or operate on tiny arrays only.

Applying the suggestions to the code

I could only rewrite the first half of the function. The second half requires a broader context to make a meaningful change to it.

As you can see, I failed to rename e into something descriptive, because I'm unaware of the models you are using in your application. Hopefully, even this "mechanical" refactoring is able to give you some ideas on how the code can be improved.

getDatasets() {
  const chartData = this.getChartData;

  const datasetLabels = Object.values(chartData);

  // `datasets` could be a const, if only it wasn't modified further down the road...
  let datasets = [...new Set(datasetLabels)]
    .map((label, labelIndex, array) => ({ label, backgroundColor: this.getColor(array, labelIndex), data: [] }));

  // This is a subject to a few more changing, maybe...
  Object
    .entries(chartData)
    .forEach((entry) => {
      Object.entries(entry[1]).forEach((dataItem) => {
          datasets = datasets.map((e) => {
            if (e.label === dataItem[0]) {
              e.data.push(dataItem[1].median);
            }
            return e;
          });
        });
    });

  return datasets;
}

Update As @Gerrit0 suggested, Object.values(...) is a better option than Object.entities(...).map(...) (more concise and achieves exactly the same result).

\$\endgroup\$
  • \$\begingroup\$ @OlgaB sure! Glad to help. \$\endgroup\$ – Igor Soloydenko May 22 '18 at 8:21
  • 1
    \$\begingroup\$ Given that you never use entry[0], it might be worth switching to Object.values \$\endgroup\$ – Gerrit0 May 22 '18 at 11:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.