Sorting tables Javascript

Question

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;
      }
    }
  }
}

vipatron · Accepted Answer · 2018-10-01 18:30:44Z

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>

Thank you for the help. I did not know I redeclared the argument. is there a way to have just the header <th> as the clickable area versus the whole column? I assume I need to change "event.target.cellIndex"
Just so we're clear, when you hang an eventListener on an element, you have 2 choices: You can define an anonymous function right there, as you sorta tried to do, or you can name it, as I did in my solution. As to your second question, I would check to see whether event.target.nodeNAME==="TH", and only call sortTable() if that were true. EDIT: Also, if you accepted my answer, would you mind upvoting it?

Collectives™ on Stack Overflow

Sorting tables Javascript

1 Answer 1

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related