5
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

Here’s another one towards using 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.

\$\endgroup\$
0
\$\begingroup\$

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

Since the code already uses .filter(), more functional approaches like .reduce() could be used to clean up the code, unless the performance losses would be too much.

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() {
      let hardErrorCodes = this.testData.filter(ts => ts.severity === 'H').map(v => v.errorCode);
      let 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());

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – redOctober13 Nov 8 '17 at 16:59
0
\$\begingroup\$

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)

\$\endgroup\$

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.