2006-09-22 15:37:50 Too many comments and brackets
This is a random piece of javascript code…
( javascript )
  1  // retrieve text of an XML document element, including
2 // elements using namespaces
3 function getElementTextNS(prefix, local, parentElem, index) {
4 var result = "";
5 if (prefix && isIE) {
6 // IE/Windows way of handling namespaces
7 result = parentElem.getElementsByTagName(prefix + ":" + local)[index];
8 } else {
9 // the namespace versions of this method
10 // (getElementsByTagNameNS()) operate
11 // differently in Safari and Mozilla, but both
12 // return value with just local name, provided
13 // there aren't conflicts with non-namespace element
14 // names
15 result = parentElem.getElementsByTagName(local)[index];
16 }
17 if (result) {
18 // get text, accounting for possible
19 // whitespace (carriage return) text nodes
20 if (result.childNodes.length > 1) {
21 return result.childNodes[1].nodeValue;
22 } else {
23 return result.firstChild.nodeValue;
24 }
25 } else {
26 return "n/a";
27 }
28 }


but is the following not what you'd rather have in your file for clarity:
( javascript )
  1  function getElementTextNS(prefix, local, parentElem, index) {
2 var result = ""
3 if (prefix && isIE)
4 result = parentElem.getElementsByTagName(prefix + ":" + local)[index]
5 else
6 result = parentElem.getElementsByTagName(local)[index]
7
8 if (result)
9 if (result.childNodes.length > 1)
10 return result.childNodes[1].nodeValue
11 else
12 return result.firstChild.nodeValue
13 else
14 return "n/a"
15 }


Three points being made here: 1) Comments can often as not muddle up the readability of code which was already fairly readable; 2) Braces around one-line code segments can make code appear visually complicated when something simple is going on (though this is a bias from someone who likes Lispy-looking code); 3) Semicolons in JavaScript is just unnecessary visual noise.

Thoughts?
  • Ben (Tue, April 28th, 2009, 9:30pm UTC)
    I'm with you. I much prefer the second one.

  • Jeff (Wed, April 29th, 2009, 12:39am UTC)
    Taking a look at this again, it strikes me that the first few lines could be made more readable and less error prone by not writing the getElementsByTagName method twice…
    ( javascript )
      1  var tagName = prefix && isIE ? (prefix + ":" + local) : local
    2 var result = parentElem.getElementsByTagName( tagName )[index]

    which may be too compact. Perhaps nicer yet:
    ( javascript )
      1  var result, tagName = local
    2 if (prefix && isIE)
    3 tagName = prefix + ":" + local
    4
    5 result = parentElem.getElementsByTagName( tagName )[index]

Leave a comment