Search code examples
htmlhtml-tabledom-eventscolumnsorting

Sorting tables Javascript


I'm sorting a table when a user clicks on the table head, and I'm using vanilla Javascript to do this. I'm trying to capture the column number by clicking the <th> table head and pass it to the sortTable(n) function. Also, I'm trying to start the sortTable(n) function after column data is captured. I'm not sure where my error is at?

document.getElementById('myTable2').addEventListener('click', function() {
  myFunction(event)
}, true);

function myFunction() {
  var col = window.event.target.cellIndex;
  var n = col;
  sortTable(n);
  return n;

}

function sortTable(n) {
  var n = myFunction();
  var table, rows, switching, i, x, y, shouldSwitch, dir, switchcount = 0;
  table = document.getElementById("myTable2");
  switching = true;
  
  dir = "asc";
  while (switching) {
   switching = false;
    rows = table.rows;
    /* Loop through all table rows (except the
    first, which contains table headers): */
    for (i = 1; i < (rows.length - 1); i++) {
      // Start by saying there should be no switching:
      shouldSwitch = false;
      /* Get the two elements you want to compare,
      one from current row and one from the next: */
      x = rows[i].getElementsByTagName("TD")[n];
      y = rows[i + 1].getElementsByTagName("TD")[n];
      /* Check if the two rows should switch place,
      based on the direction, asc or desc: */
      if (dir === "asc") {
        if (x.textContenttextContent.toLowerCase() > y.textContent.toLowerCase()) {
          shouldSwitch = true;
          break;
        }
      } else if (dir === "desc") {
        if (x.textContent.toLowerCase() < y.textContent.toLowerCase()) {
          // If so, mark as a switch and break the loop:
          shouldSwitch = true;
          break;
        }
      }
    }
    if (shouldSwitch) {
      rows[i].parentNode.insertBefore(rows[i + 1], rows[i]);
      switching = true;
      // Each time a switch is done, increase this count by 1:
      switchcount++;
    } else {
       if (switchcount === 0 && dir === "asc") {
        dir = "desc";
        switching = true;
      }
    }
  }
}


Solution

  • Phew. There were a fair amount of coding problems in the way you called the eventHandler "myFunction()". There were also some very dangerous habits you were falling into regarding variable scope. I rewrote those. On the upside, your application logic was sound. Good job! I wrote a pen that works to show it to you in action if the snippet doesn't. But, follow my advice about changing variable names, especially if this is for an assignment.

    document.getElementById('myTable2').addEventListener('click', myFunction, true);
    
    function myFunction(event) {
      // window.alert("In myFunction");
      // console.log(event);
      
      var col = event.target.cellIndex;
      //Following line was redundant;
      //var n = col;
      sortTable(col);
      return col;
    
    }
    
    
    //PIECE OF ADVICE: If you renamed the variable "n" to "selectedCol", it would be more readable. Or at least "col"
    function sortTable(n) {
      // WHY are you redeclaring the argument?
      // And why are you assigning myFunction, which called this function, as the object which you are assigning to n?
    //   var n = myFunction();
    
      // OLD VERSION: All declared and set to 0, then later reassigned: Confusing and can get you into problems with variable scope, and unused declarations. 
      // var table, rows, switching, i, x, y, shouldSwitch, dir, switchcount = 0;
      var switchcount = 0;
      
      var table = document.getElementById("myTable2");
      var switching = true;
      var dir = "asc";
    
      while (switching) {
       switching = false;
       var rows = table.rows;
        var shouldSwitch;
    
        /* Loop through all table rows (except the
        first, which contains table headers): */
        for (var i = 1; i < (rows.length - 1); i++) {
          // Start by saying there should be no switching:
          shouldSwitch = false;
          /* Get the two elements you want to compare,
          one from current row and one from the next: */
          var x = rows[i].getElementsByTagName("TD")[n];
          var y = rows[i + 1].getElementsByTagName("TD")[n];
          /* Check if the two rows should switch place,
          based on the direction, asc or desc: */
          if (dir === "asc") {
            if (x.textContent.toLowerCase() > y.textContent.toLowerCase()) {
              shouldSwitch = true;
              break;
            }
          } else if (dir === "desc") {
            if (x.textContent.toLowerCase() < y.textContent.toLowerCase()) {
              // If so, mark as a switch and break the loop:
              shouldSwitch = true;
              break;
            }
          }
        }
        if (shouldSwitch) {
          rows[i].parentNode.insertBefore(rows[i + 1], rows[i]);
          switching = true;
          // Each time a switch is done, increase this count by 1:
          switchcount++;
        } else {
           if (switchcount === 0 && dir === "asc") {
            dir = "desc";
            switching = true;
          }
        }
      }
    }
    table, th, td {
        border: 1px solid black;
        border-collapse: collapse;
    }
    th, td {
        padding: 5px;
    }
    th {
        text-align: left;
    }
    <table id = "myTable2" style="width:100%">
      <tr>
        <th>Firstname</th>
        <th>Lastname</th> 
        <th>Age</th>
      </tr>
      <tr>
        <td>Jill</td>
        <td>Smith</td>
        <td>50</td>
      </tr>
      <tr>
        <td>Eve</td>
        <td>Jackson</td>
        <td>94</td>
      </tr>
      <tr>
        <td>John</td>
        <td>Doe</td>
        <td>80</td>
      </tr>
    </table>