sangrila B sangrila B - 7 months ago 37
Javascript Question

Best practice: functions within loops

I got the following code which works perfectly. What it does is: in a table it highlights the corresponding table header cell and table first column cell when you hover over any table cell.

// Row & Column Highlight
(function() {

var gridCellRow = null,
gridCellCol = null,
tableElement = document.getElementsByClassName('inner_table');
for (var i = 0, len_i = tableElement.length; i < len_i; i++) {
if (tableElement[i].getElementsByClassName('row_label_cell').length > 0) {
var gridCell = tableElement[i].getElementsByClassName('input_cell');
for (var j = 0, len_j = gridCell.length; j < len_j; j++) {
function gridCellParents(currentCell) {
return gridCellRow = currentCell.parentNode.firstElementChild,
gridCellCol = currentCell.parentNode.parentNode.rows[0].cells[currentCell.cellIndex];
}
gridCell[j].addEventListener('mouseover', (function() {
gridCellParents(this);
gridCellRow.classList.add('highlight');
gridCellCol.classList.add('highlight');
}));
gridCell[j].addEventListener('mouseout', (function() {
gridCellRow.classList.remove('highlight');
gridCellCol.classList.remove('highlight');
}));
}
}
}

}());


However, JSHint tells me, that

for (var j = 0, len_j = gridCell.length; j < len_j; j++) {
function gridCellParents(currentCell) {
return gridCellRow = currentCell.parentNode.firstElementChild,
gridCellCol = currentCell.parentNode.parentNode.rows[0].cells[currentCell.cellIndex];
}


is not best practice "Function declarations should not be placed in blocks. Use a function expression or move the statement to the top of the outer function."

as well as

gridCell[j].addEventListener('mouseover', (function() {
gridCellParents(this);
gridCellRow.classList.add('highlight');
gridCellCol.classList.add('highlight');
}));
gridCell[j].addEventListener('mouseout', (function() {
gridCellRow.classList.remove('highlight');
gridCellCol.classList.remove('highlight');
}));
}


is not best practice "Don't make functions within a loop."

So how am I correctly and according to best practice building this whole function?

Answer

That would be a good starting point. more ordered code:

     // Row & Column Highlight
(function() {
    var gridCellRow,
        gridCellCol,
        gridCell,
        tableElement = document.getElementsByClassName('inner_table');   

    function gridCellParents(currentCell) {
        gridCellRow = currentCell.parentNode.firstElementChild,
        gridCellCol = currentCell.parentNode.parentNode.rows[0].cells[currentCell.cellIndex];
    }

    function onMouseEnter() {
        gridCellParents(this);
        gridCellRow.classList.add('highlight');
        gridCellCol.classList.add('highlight');
    }

    function onMuoseLeave() {
        gridCellRow.classList.remove('highlight');
        gridCellCol.classList.remove('highlight');
    }


    for (var i = 0, len_i = tableElement.length; i < len_i; i++) {
        if (tableElement[i].getElementsByClassName('row_label_cell').length > 0) {
            gridCell = tableElement[i].getElementsByClassName('input_cell');                    
            for (var j = 0, len_j = gridCell.length; j < len_j; j++) {
                gridCell[j].addEventListener('mouseenter', onMouseEnter);
                gridCell[j].addEventListener('mouseleave', onMuoseLeave);
            }
        }
}}());

As you can see, I've modified your events to mousenter and mouseleave which might better suit your needs and be better for overall performance.


Update - delegated version:

 // Row & Column Highlight
(function() {
    var gridCell,
        tableElement = document.querySelectorAll('.inner_table');   

    function getCellParents(cell){
        return {
            row : cell.parentNode.firstElementChild,                       // row
            col : cell.parentNode.parentNode.rows[0].cells[cell.cellIndex] // col
       }; 
    }

    function updateGridCellParents(cell, state) {
        state = state ? 'add' : 'remove';

        var parents = getCellParents(cell);

        parents.row.classList[state]('highlight');
        parents.col.classList[state]('highlight');
    }

    funciton checkTarget(target){
        // make sure the element is what we expected it to be
        return target.className.indexOf('input_cell') != 0;
    }

    function onMouseEvents(e){
        checkTarget(e.target) && updateGridCellParents(e.target, e.type == "mouseover");
    }

    document.body.addEventListener('mouseover', onMouseEvents);
    document.body.addEventListener('mouseout', onMouseEvents);
})();