5
\$\begingroup\$

I have a function that adds a class name to a DOM element is met. The script provided works.

function rm_row(){ 

  var chk_inherent =  arcapi.dataResult().columnNameToIndex('Inherent Risk');
  var chk_residual = arcapi.dataResult().columnNameToIndex('Residual Risk')
  var chk_perf = arcapi.dataResult().columnNameToIndex(arcapi.getSetting('Column name'));

  d3.selectAll("#" + arcapi.chartId() + " .dataTables_scrollBody tbody tr").classed('removerow', function() {

    const perf = this.children[chk_perf].innerHTML
    const inherent = this.children[chk_inherent].innerHTML
    const residue = this.children[chk_residual].innerHTML
    const cf_regex = perf.replace(/[`~%]/gi, '');

    //  **** Check *****
    const inh_isEmpty = inherent === '' || inherent === 'Null'
    const rsk_isEmpty = residue === '' || residue === 'Null'
    const perf_isInvalid = cf_regex < 0 || cf_regex > 100

    /** Logic [if inherent is empty AND  Residual risk is empty AND is perf value is incorrect = True] **/
    return inh_isEmpty && rsk_isEmpty && perf_isInvalid === true
  });
}

However, I feel it's long-winded. I feel this can be further optimized. What is the best approach/best practices to further improve myself and optimize my script so that it will be easy to maintain?

I have thought of creating an array and store all my variables in an array, but I feel this approach will be expensive.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ What do you mean by "...DOM element is met" (seems incomprehensible). E.g., is a word missing? \$\endgroup\$ Sep 9, 2020 at 15:22

2 Answers 2

11
\$\begingroup\$

However, I feel it's long-winded.

To reduce the amount of repeated code, and to make it easier to adapt for different columns without writing the same sort of thing again 2 or 3 times, look for similarities across the different columns. For every column, you're calling arcapi.dataResult().columnNameToIndex, so you can make a function which, given a column name, calls that. You're also accessing the this.children[index].innerHTML for every index retrieved.

One option is to use objects instead of multiple standalone variables. You can have an object indexed by variable names, whose values are the column indicies. Then, in the selectAll callback, you can construct an object with the same properties but with the text values you're interested in by using mapping the object's entries and passing into Object.fromEntries:

function rm_row() {
  const getIndex = columnName => arcapi.dataResult().columnNameToIndex(columnName);
  const columnIndiciesByName = {
    inherent: getIndex('Inherent Risk'),
    residual: getIndex('Residual Risk'),
    perf: getIndex(arcapi.getSetting('Column name')),
  };

  d3.selectAll("#" + arcapi.chartId() + " .dataTables_scrollBody tbody tr").classed('removerow', function() {
    const columnTextsByName = Object.fromEntries(
      Object.entries(columnIndicies).map(
        ([key, columnIndex]) => ([key, this.children[columnIndex].innerHTML])
      )
    );
    columnTextsByName.perf = columnTextsByName.perf.replace(/[`~%]/gi, '');
    // Then refer to columnTextsByName.inherent, etc

The above is just an option to consider. While this makes the code less repetitive and more flexible for future added or removed columns you might want to check, I'm not entirely convinced that it's a solid improvement over your current code if you're not expecting your current code to change. YMMV; you may consider your current code to be more readable, or not.

Other possible improvements:

Don't use var in ES6: If you're going to write in ES6 syntax - which you should, it's great - there's no reason to use var, it has too many gotchas (such as function-scope hoisting and automatically being assigned to the global object on the top level) to be worth using. Always use const (or, when you must reassign the variable, let).

JS naming conventions The vast majority of professional JavaScript uses camelCase for functions and ordinary variables. snake_case is quite rare (and mixing snake case with camelCase in the same variable name is even weirder). Consider using camelCase everywhere that it's appropriate.

cf_regex On a similar note, name your variables appropriately: in your original code, cf_regex is not a regular expression, it's just a string which has had certain characters replaced. Better to call it something more accurate and longer (I don't have any idea what it's meant to be, and neither will other readers of the code given only this context)

Something odd about the regex - you're using the case-insensitive flag i, but you're not matching any letters, so the flag isn't doing anything. Might as well remove it. Or, to make it clearer what's going on, if you're expecting the result to contain only numbers, match digits instead:

columnTextsByName.perf = Number(columnTextsByName.perf.match(/\d+/)[0]);

(since you're going to be comparing with numbers later, it feels better to cast the variable being compared to a number too; it makes more sense and will make debugging a bit easier if problems arise later)

innerHTML or textContent? Unless you're deliberately retrieving HTML markup, which doesn't look present here, if you just care about the text of a cell, it would be more appropriate to use .textContent rather than .innerHTML. It's faster too.

Semicolons You're using a few semicolons, but also missing a bunch. Unless you're an expert or use semicolons everywhere, you may eventually get tripped up by Automatic Semicolon Insertion. Consider using a linter.

Performance You say

I have thought of creating an array and store all my variables in an array, but I feel this approach will be expensive.

On modern computers, the overhead of creating an array or object instead of multiple standalone variables is completely nonexistent. If using a particular data structure makes the code more maintainable (given whatever style you prefer), go ahead and do it. If you later find that there's a performance issue, feel free to go back and debug to find what exactly the bottleneck is, so you can figure out a more efficient method - but using an object or array will almost certainly not be the bottleneck. Try to avoid premature optimization, or at least don't sacrifice code clarity for it unless you have to.

\$\endgroup\$
4
  • \$\begingroup\$ The question has been updated. Can you update the quoted parts? \$\endgroup\$ Sep 9, 2020 at 15:23
  • \$\begingroup\$ How can you tell that it's ES6? I am aware of ES6 but I've only used ES5 so far (IE support reqs, only recently removed) and I cannot see anything non-ES5 except that there are some missing semicolons? \$\endgroup\$ Sep 9, 2020 at 15:46
  • 3
    \$\begingroup\$ @RBarryYoung He is using const, and const is ES6 syntax. In ES5 and before, there was only var to declare variables. \$\endgroup\$ Sep 9, 2020 at 15:47
  • \$\begingroup\$ Ah thanks. I guess I didn't realize that const was not ES5 (though it would explain why I couldn't get it to work on IE and thus stopped using it). \$\endgroup\$ Sep 9, 2020 at 15:49
1
\$\begingroup\$

Refactor repetitive code into reusable functions.

The first thing that popped out to me was this pair of repetitive lines:

const inh_isEmpty = inherent === '' || inherent === 'Null'
const rsk_isEmpty = residue === '' || residue === 'Null'

Let's turn that into a function instead:

function isEmpty(string) {
  return string === '' || string === 'Null'
}

Now we can get rid of the inh_isEmpty and rsk_isEmpty variables entirely, and just write isEmpty(inherent) and isEmpty(residue) instead. That may not be a huge saving in terms of raw line count, but it certainly looks cleaner. And you might be able to reuse the isEmpty function elsewhere in your code, too.

But we're not done yet. These lines also look very repetitive:

var chk_inherent =  arcapi.dataResult().columnNameToIndex('Inherent Risk');
var chk_residual = arcapi.dataResult().columnNameToIndex('Residual Risk')
var chk_perf = arcapi.dataResult().columnNameToIndex(arcapi.getSetting('Column name'));

(BTW, why are you using var here but const/let elsewhere? There's very little point in mixing these two styles of variable declarations. Be consistent! For that matter, your semicolon usage is also kind of random.)

…as do these lines below:

const perf = this.children[chk_perf].innerHTML
const inherent = this.children[chk_inherent].innerHTML
const residue = this.children[chk_residual].innerHTML

One option would be to refactor the repetitive parts of these lines into a function like this:

function getColumnHTML(row, columnName) {
  const index = arcapi.dataResult().columnNameToIndex(columnName)
  return row.children[index].innerHTML
}

…and use it like e.g. this:

const perf = getColumnHTML(this, arcapi.getSetting('Column name'))
const inherent = getColumnHTML(this, 'Inherent Risk')
const residual = getColumnHTML(this, 'Residual Risk')

(BTW, I renamed your residue variable to residual to match the column name.)

While we're at it, let's define a function for your validity check too:

function isValidPercentage(string) {
  const percent = Number(string.replace(/[`~%]/g, ''))
  return percent >= 0 && percent <= 100
}

(JavaScript string-to-number comparison can be tricky, so I'd prefer explicitly running the filtered string through Number(). In particular, doing so ensures that any unparseable inputs get turned into NaN, which then fails the comparison. Also, the /i switch is useless for this regexp, so I removed it.)

With all these changes, your refactored code would look something like this:

function getColumnHTML(row, columnName) {
  const index = arcapi.dataResult().columnNameToIndex(columnName)
  return row.children[index].innerHTML
}

function isEmpty(string) {
  return string === '' || string === 'Null'
}

function isValidPercentage(string) {
  const percent = Number(string.replace(/[`~%]/gi, ''))
  return percent >= 0 && percent <= 100
}

function removeBadRows() { 
  d3.selectAll("#" + arcapi.chartId() + " .dataTables_scrollBody tbody tr").classed('removerow', function() {
    const perf = getColumnHTML(this, arcapi.getSetting('Column name'))
    const inherent = getColumnHTML(this, 'Inherent Risk')
    const residual = getColumnHTML(this, 'Residual Risk')

    return isEmpty(inherent) && isEmpty(residual) && !isValidPercentage(perf)
  })
}

Of course, you can also choose to move the helper functions somewhere else — e.g. inside the removeBadRows function, if you don't want to have them visible outside it, or alternatively into some reusable collection of utility functions.

Ps. A few other changes I'd also suggest:

  • If you don't specifically need the raw HTML code (and it looks like you don't), use textContent (or its effective synonym innerText) instead of innerHTML to access the text inside a DOM element. It's both easier (no need to worry about HTML parsing or entity decoding) and safer (less opportunities for accidental HTML injection bugs).

  • Use a stricter regexp match to validate the percentages. I can't suggest a specific regexp since I don't know exactly what your data looks like, but just as an example, if you only ever had an unsigned (integer or decimal) number followed by a % sign, you could do something like:

    function isValidPercentage(string) {
      const match = /^([0-9]+)(\.[0-9]+)?%$/.exec(string)
      if (!match) return false
      const percent = Number(match[1] + match[2])
      return percent >= 0 && percent <= 100
    }
    
  • Building DOM selectors via string concatenation is ugly and bug-prone. Avoid it if you can, minimize it if you can't. Consider e.g. doing something like:

    const chart = d3.select('#' + arcapi.chartId())
    chart.selectAll('.dataTables_scrollBody tbody tr').classed('removerow', // ...
    

    or even:

    const chart = d3.select(document.getElementById(arcapi.chartId()))
    // ...
    

    (and consider saving the chart variable as a global constant, or wrap the code for computing it into yet another helper function to avoid repeating it).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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