1
\$\begingroup\$

I have a JavaScript code that gets the inner text content of certain table cells in a row and displays them as a tooltip on mouseover. However, I am a novice at JavaScript and I do not know how to make the code more efficient and/or secure from exploits.

<!DOCTYPE html>
<html lang='en'>

<head>
<meta charset='utf-8' />
<title>Test page</title>
</head>

<body>

<table id='table1'>
<thead>
<tr>
<th>Name</th>
<th>Population (2000)</th>
<th>Population (2005)</th>
</tr>
</thead>
<tbody>
<tr>
<td id='placeName'>San Antonio</td>
<td>3,420</td>
<td>3,572</td>
</tr>
<tr>
<td id='placeName'>Green Valley</td>
<td>1,409</td>
<td>1,222</td>
</tr>
</tbody>
</table>
<script>
function placeTitle(row) {
elemRow = row.children[0];
row.title=elemRow.innerHTML;
}
var hoverRow = document.querySelectorAll("#table1 tbody tr"),
trLen = hoverRow.length;


var place = document.getElementById('placeName');

function placeTitle(row) {
elem = row.children[0];
elem1 = row.children[1];
elem2 = row.children[2];
row.children[0].title=elem.textContent;

elem1.title=elem.textContent + '\nPopulation (2000): ' + elem1.textContent;
elem2.title=elem.textContent + '\nPopulation (2005): ' + elem2.textContent;
}

for (i = 0; i < trLen; i++) {
hoverRow[i].setAttribute("onmouseover","javascript: placeTitle(this);");
}
</script>
</body>

</html>

 

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

As far as making the code "secure from exploits", that might be tough to do with JavaScript, unless you take tactics like minification/obfuscation, etc. The suggestions below should allow the code to be more robust.

Naming

elem, elem1 and elem2 aren't very descriptive. Better names might be nameElement, populationElement1, populationElement2, etc.

Additionally, the variable in the for loop (i.e. i) is not declared using var so it is put into the global namespace.

Allow number of population columns to be dynamic

Currently the code only allows two columns of populations, and hard-coded to years 2000 and 2005. The table headers could have a class added:

<th>Name</th>
<th class="populationHeader">Population (2000)</th>
<th class="populationHeader">Population (2005)</th>

Then those population headers can be fetched from the DOM using document.getElementsByClassName() and stored in a variable:

var populationHeaders = document.getElementsByClassName('populationHeader');

Then the function placeTitle() can iterate over those titles and add the title attributes to the elements using the iterator variable, eliminating the need for elem1 and elem2:

for (var k = 0; k < populationHeaders.length; k++) {
    var populationCell = row.children[k+1];
    populationCell.title = nameElement.textContent + '\n' + populationHeaders[k].textContent+':' + populationCell.textContent;
}

Setting the titles on mouseover?

Why call placeTitle() when the use mouses over each row? This can lead to calling the function hundreds of times when it really only needs to be called once. So instead of the following:

for (i = 0; i < trLen; i++) {
    hoverRow[i].setAttribute("onmouseover","javascript: placeTitle(this);");
}

It would be better to just call placeTitle() once per row (instead of on every mouse over event):

for (i = 0; i < trLen; i++) {
    placeTitle(hoverRow[i]);
}

Or this could be done with the function approach - using Array.forEach():

hoverRow.forEach(placeTitle);

Duplicate function

Your code has two functions called placeTitle(). The first gets overwritten by the second, thus making it useless. Make sure your function names are unique.

var hoverRow = document.querySelectorAll("#table1 tbody tr"),
  trLen = hoverRow.length;


var place = document.getElementById('placeName');
var populationHeaders  = document.getElementsByClassName('populationHeader');

function placeTitle(row) {
  nameElement= row.children[0];
  nameElement.title = nameElement.textContent;
  for (var k = 0; k < populationHeaders.length; k++) {
    var populationCell = row.children[k+1];
    populationCell.title = nameElement.textContent + '\n' + populationHeaders [k].textContent+':' + populationCell.textContent;
  }
}

for (i = 0; i < trLen; i++) {
  placeTitle(hoverRow[i]);
}
<table id='table1'>
  <thead>
    <tr>
      <th>Name</th>
      <th class="populationHeader">Population (2000)</th>
      <th class="populationHeader">Population (2005)</th>
    </tr>
  </thead>
  <tbody>
    <tr>
      <td id='placeName'>San Antonio</td>
      <td>3,420</td>
      <td>3,572</td>
    </tr>
    <tr>
      <td id='placeName'>Green Valley</td>
      <td>1,409</td>
      <td>1,222</td>
    </tr>
  </tbody>
</table>

Functional approach to iterating over the headers

In the snippet below, the headers are iterated over using Array.forEach(). That way there is no need to index into the array of population headers. document.getElementsByClassName() returns a HTMLCollection interface (different than an array) so the spread syntax (i.e. ...) is used to put the elements into an array.

var hoverRow = document.querySelectorAll("#table1 tbody tr"),
  trLen = hoverRow.length;


var place = document.getElementById('placeName');
var populationHeaders = document.getElementsByClassName('populationHeader');

function placeTitle(row) {
  nameElement= row.children[0];
  nameElement.title = nameElement.textContent;
  [...populationHeaders].forEach(function(populationHeader, index) {
    var populationCell = row.children[index + 1];
    populationCell.title = nameElement.textContent + '\n' + populationHeader.textContent + ':' + populationCell.textContent;
  });
}
hoverRow.forEach(placeTitle);
<table id='table1'>
  <thead>
    <tr>
      <th>Name</th>
      <th class="populationHeader">Population (2000)</th>
      <th class="populationHeader">Population (2005)</th>
    </tr>
  </thead>
  <tbody>
    <tr>
      <td id='placeName'>San Antonio</td>
      <td>3,420</td>
      <td>3,572</td>
    </tr>
    <tr>
      <td id='placeName'>Green Valley</td>
      <td>1,409</td>
      <td>1,222</td>
    </tr>
  </tbody>
</table>

For more tips on interacting with the DOM via Javascript, check out this article. I know it is a few years old but still relevant*.

*at the time of writing

\$\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.