Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is loosely based on one of W3_School samples:

<!DOCTYPE html>
<html>
<body>

<p>Hello World!</p>
<p>The DOM is very useful!</p>
<p>This example demonstrates forEach() function</p>

<script>
var x=Array.prototype.slice.call(document.getElementsByTagName("p"));
x.forEach(function(value, index)
  { 
  document.write(value.innerHTML);
  document.write("<br>");
  });
</script>

</body>
</html>

Please let me know if any improvements need (could) be done to the script.

share|improve this question

2 Answers 2

up vote 11 down vote accepted

The only purpose of this script appears to be to illustrate the use the Array.prototype.slice.call "idiom" and the forEach function. So it's hard to make "improvements" when the purpose of the code isn't its actual result (which is trivial).

What the code actually does can be done in a number of ways, some of which won't (directly) use forEach and/or NodeList and/or slice.call. In that case, is it still an improvement? Conversely, sticking to forEach etc. doesn't leave much room for change, since the code seems designed as a minimal example of those things.

I'm hesitant to write any code, because I'm not sure what the point would be.

With that said,

  • Don't rely on W3 Schools (this site lists several reasons and better alternatives)
  • Avoid document.write() - or use it with open eyes (see why here)
  • I'd also avoid forEach in favor of a standard for loop, since forEach is not universally supported in browsers (see caniuse.com)
  • The page's text appears to betray some confusion. Yes, the DOM is useful, and yes, the code demonstrates forEach. But forEach isn't part of the DOM. The whole point is that a NodeList object is a DOM object and not a normal array. It must therefore be converted to one by way of Array.prototype.slice.call. Then one can use forEach which is an array function but which, in and of itself, has nothing to do with the DOM.

Update after reading @PM77-1's comment:

There's nothing inherently wrong with this code. It is true, that
A) for...in doesn't behave like it does in Java and shouldn't be used here, and that
B) getElementsByTagName doesn't return a "real" array. The slice.call trick is indeed the "standard" way of converting an "array-like" object (which you find a lot of, unfortunately) to a real array. A very useful trick to know.

So all in all, the code is basically fine. Yes, improvements can be made, as @Palacsint points out in his answer, but there's nothing terrible going on.

However, while a NodeList object isn't a real array, one can still use a regular old for loop:

var nodes, i;

nodes = document.getElementsByTagName("p");
for(i = 0 ; i < nodes.length ; i++) {
  document.write(nodes[i].innerHTML);
  document.write("<br>");
}

This removes the need for the slice.call trick, and it works on old browsers (where forEach doesn't exist).

In general, though, when working with the DOM, it's useful to use a library to iron out the otherwise frustrating differences between browsers. jQuery is the king of the hill these days, and with that, you'd do something like this to find and iterate over some elements:

$("p").each(function () {
  document.write(this.innerHTML);
  document.write("<br>");
});

(Of course, jQuery also makes it easy to do "proper" DOM insertion without resorting to document.write, but this is just for illustration)

However, I do think it very wise to learn the nitty-gritty of JS, and not just run straight for jQuery or its ilk. And dealing with those "array-like" is one of those nitty-gritty things indeed.

share|improve this answer
    
It was actually my first JavaScript code. Originally I wanted to use for-in (assuming that it's similar to one in Java) instead of the regular for loop in one of W3 samples. After I realized that it's a completely different animal and does not iterate through a collection, I tried to apply forEach() function to the return of getElementsByTagName() assuming (wrongly again) that it returns an array. At the end I had to use Array.prototype.slice.call to convert the list to array. The resulting code looks somewhat convoluted, so I asked this question. –  PM 77-1 Mar 16 at 22:31
1  
@PM77-1 Well, your code is about as good as it gets given what you aimed for. I know it looks convoluted, but, well, that's because the DOM is an odd beast. I've added some more to my answer. –  Flambino Mar 16 at 23:22

Just two minor notes to add @Flambino's great answer:

  1. Instead of x use intention-revealing names. What's the purpose of this variable? Name according to that. paragraphs would be a better name here, it explains the purpose of the variable and avoids mental mapping (readers/maintainers don't have to decode it every time). (It's just a minor thing here, everyone has one slot of short-term memory for x but having a couple of abbreviations in a larger project could make understanding code really hard.)

    (Clean Code by Robert C. Martin, Use Intention-Revealing Names, p18; Avoid Mental Mapping, p25)

  2. W3C Validator founds one error and at least one warning. You could fix those by setting a title and an encoding:

    <!DOCTYPE html>
    <html>
    <head>
        <title>DOM test page</title>
        <meta charset="UTF-8">
    </head>
    <body>
    ...
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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