Aggregate summary counts for an array of error objects

Question

I'm doing some refactoring of someone else's code, and just want a second opinion, because of course I think my work makes it better, but some validation (or correction) would be helpful.

Starting with an array like this:

errorLog: [{
 errorCode: 11,
 errorDescription: "abc",
 date: "2017-01-01",
 severity: "H"
},{
 errorCode: 11,
 errorDescription: "abcd",
 date: "2017-01-02",
 severity: "H"
},{
 errorCode: 99,
 errorDescription: "abcd",
 date: "2017-01-02",
 severity: "H"
}]

and trying to get results like this:

errorSummary: [{
 errorCode: 11,
 severity: "H",
 count: 2
},{
 errorCode: 99,
 severity: "H",
 count: 1
}]

this is the existing code:

//instead of this, which is hard to reason about and debug (and includes a line that will never rturn true: if (hardErrorsSorted.includes...)):
let hardErrors = testData.filter(ts1 => ts1.severity === 'H');
let hardErrorsSorted = hardErrors.sort(this.mySorter);
for (let i = 0; i < hardErrorsSorted.length; i++) {
 if (i != hardErrorsSorted.length - 1) {
 if (hardErrorsSorted[i].errorCode != hardErrorsSorted[i + 1].errorCode) {
 let errorCount = this.getCount(hardErrorsSorted, hardErrorsSorted[i].errorCode);
 this.errorDataList.push({
 errorCode: hardErrorsSorted[i].errorCode,
 errorCodeType: 'H',
 errorCodeTotalCount: errorCount
 });
 }
 } else {
 if (hardErrorsSorted.includes(hardErrorsSorted[i].errorCode, 0)) {
 } else {
 let errorCount = this.getCount(hardErrorsSorted, hardErrorsSorted[i].errorCode);
 this.errorDataList.push({
 errorCode: hardErrorsSorted[i].errorCode,
 errorCodeType: 'H',
 errorCodeTotalCount: errorCount
 });
 }
 }
}

and my refactoring:

//use something like this, which is much easier to grasp at a glance, doesn't jump around, and is DRYer
let hardErrorCodes = testData.filter(ts => ts.severity === 'H').map(v => v.errorCode);
let hardErrorCounts = {};
//sum up the unique errors
for (let error of hardErrorCodes) {
 if (!(error in hardErrorCounts)) {
 hardErrorCounts[error] = 0;
 }
 hardErrorCounts[error]++;
}
//add the summed error counts to the master list
for (let error in hardErrorCounts) {
 this.errorDataList.push({
 errorCode: error, 
 errorCodeType: "H", 
 errorCodeTotalCount: hardErrorCounts[error]
 });

What do you all think? Is this a helpful refactor, or a waste of time?

Answer 1

Here’s another one towards functional using es6 syntax additions:

const predSeverity = severity => err => err.severity == severity
const byCode = (acc, {errorCode: code}) =>
 acc.set(code, (acc.get(code) || 0) + 1)
errorLog.filter(predSeverity("H")).reduce(byCode, new Map())

It has a bit different result data format (and type, if only for the receiver-as-return-value of Map::set), since the filtering already tells us the severity.

Answer 2

I had posted first on SO; then this was the answer given by another user, which I quite like, and whereas mine would need to have more code for warnings, this one doesn't:

You could even go further:

const result = [], ids = {};
for(const {errorCode, severity} of hardErrorCodes){
 let uniqueId = errorCode + severity;
 if(ids[uniqueId]){
 ids[uniqueId].count++;
 } else {
 result.push( ids[uniqueId] = { severity, errorCode, count: 1});
 }
}

This makes it 3 times faster (in theory)

Answer 3

The rewritten code definitely feels less cluttered, and it makes sense to remove a block that never gets executed...

There are places where const could be used instead of let - e.g. for errorCount , since it never gets re-assigned. It is advised to default to using const and then switch to let when re-assignment is deemed necessary. This helps avoid accidental re-assignment and other bugs.

Since the code already uses .filter(), more functional approaches like using the .reduce() method could be used to simplify iterating through the list, unless the performance losses would be too much. Instead of using filter and map, reduce could be used to combine the two approaches - conditionally adding to an output array/object.

function ErrorLog() {
 return {
 testData: [{
 errorCode: 11,
 errorDescription: "abc",
 date: "2017-01-01",
 severity: "H"
 }, {
 errorCode: 11,
 errorDescription: "abcd",
 date: "2017-01-02",
 severity: "H"
 }, {
 errorCode: 99,
 errorDescription: "abcd",
 date: "2017-01-02",
 severity: "H"
 }],
 errorDataList: [],
 getDataList: function() {
 const hardErrorCodes = this.testData.filter(ts => ts.severity === 'H').map(v => v.errorCode);
 const hardErrorCounts = hardErrorCodes.reduce(function(counts, error) {
 counts[error] = counts[error] ? counts[error] + 1 : 1;
 return counts;
 }, {});
 return Object.keys(hardErrorCounts).map(function(error) {
 return {
 errorCode: error,
 errorCodeType: "H",
 errorCodeTotalCount: hardErrorCounts[error]
 };
 });
 }
 };
}
const log = ErrorLog();
console.log(log.getDataList());

• redOctober13: Thanks. I had thought of using reduce as well, but didn't nail it down before I came up with my first solution. The downside of this, as with my initial refactoring, is that you have to duplicate most of the code for summing warnings (severity = "W"), whereas the answer above doesn't require that, and only makes one trip through a loop. Curious to know what you think about that one.
• Sᴀᴍ Onᴇᴌᴀ: Sorry for not responding sooner! I support minimizing the number of times data is looped over... which is one reason I would support using reduce instead of filter and map in some cases.