HJ Blog > Code Snippet #51: Too many comments and brackets

2006-09-22 15:37:50 Too many comments and brackets

This is a random piece of javascript code…
( javascript ) ✂

// retrieve text of an XML document element, including
// elements using namespaces
function getElementTextNS(prefix, local, parentElem, index) {
    var result = "";
    if (prefix && isIE) {
        // IE/Windows way of handling namespaces
        result = parentElem.getElementsByTagName(prefix + ":" + local)[index];
    } else {
        // the namespace versions of this method 
        // (getElementsByTagNameNS()) operate
        // differently in Safari and Mozilla, but both
        // return value with just local name, provided 
        // there aren't conflicts with non-namespace element
        // names
        result = parentElem.getElementsByTagName(local)[index];
    }
    if (result) {
        // get text, accounting for possible
        // whitespace (carriage return) text nodes 
        if (result.childNodes.length > 1) {
            return result.childNodes[1].nodeValue;
        } else {
            return result.firstChild.nodeValue;            
        }
    } else {
        return "n/a";
    }
}

  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 ) ✂

function getElementTextNS(prefix, local, parentElem, index) {
    var result = ""
    if (prefix && isIE)
        result = parentElem.getElementsByTagName(prefix + ":" + local)[index]
    else
        result = parentElem.getElementsByTagName(local)[index]

    if (result)
        if (result.childNodes.length > 1)
            return result.childNodes[1].nodeValue
        else
            return result.firstChild.nodeValue        
    else
        return "n/a"
}

  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?

RSS Feed for this page

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 ) ✂

var tagName = prefix && isIE ? (prefix + ":" + local) : local
var result = parentElem.getElementsByTagName( tagName )[index]

  1 var tagName = prefix && isIE ? (prefix + ":" + local) : local
  2 var result = parentElem.getElementsByTagName( tagName )[index]

which may be too compact. Perhaps nicer yet:
( javascript ) ✂

var result, tagName = local
if (prefix && isIE)
    tagName = prefix + ":" + local

result = parentElem.getElementsByTagName( tagName )[index]

  1 var result, tagName = local
  2 if (prefix && isIE)
  3     tagName = prefix + ":" + local
  4 
  5 result = parentElem.getElementsByTagName( tagName )[index]