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

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).