Case study #3

I looked at a random website JS code and discovered the following

/*** Part 1 ***/
function get(id) { return document.getElementById(id); }
function getByTagName(tag) { return document.getElementsByTagName(tag); }

/*** Part 2 ***/
function getByClassName(clsName) {
    var retVal = new Array();
    var elements = document.getElementsByTagName("*");
    for(var i = 0;i < elements.length;i++){
        if(elements[i].className.indexOf(" ") >= 0){
            var classes = elements[i].className.split(" ");
            for(var j = 0;j < classes.length;j++){
                if(classes[j] == clsName)
                    retVal.push(elements[i]);
            }
        }
        else if(elements[i].className == clsName)
            retVal.push(elements[i]);
    }
    return retVal;
}

/*** Part 3 ***/
function exists(o) { 
	try {
		if((typeof(o) !== "undefined")&&(o!=undefined)&&(o!=null)&&(o!="null")) { return true; } else { return false; } 
	} catch(error){ return false; }
}

/*** Part 4 ***/
function getInnerHTML(id) { return document.getElementById(id).innerHTML; }
function setInnerHTML(id, d) { var e = get(id); if (e != null) { e.innerHTML = d; } }
function getClassName(id) { return document.getElementById(id).className; }
function setClassName(id, c) {  var e = get(id);if (e != null) { e.className = c; } }
function getStyleAttr(id,p) { var e = get(id);if(e != null) { return e.style[p]; } }
function setStyleAttr(id,p,v) { var e = get(id);if(e != null) { e.style[p] = v; } }
function changeCLS(id,cls) { setClassName(id,cls); }
function formatURL(url){ var formattedURL = unescape(url);window.location = formattedURL;}

/*** Part 5 ***/
function show(id) {
	var display = (arguments.length > 1) ? arguments[1] : "block";
	if (typeof(id) === "string" ) {
		var e = get(id);
		if (e != null) {
			setStyleAttr(id,"display",display);
		}
	} else {
		for (var i=0, j=id.length; i<j; i++) {
			var e = get(id[i]);
			if (e != null) {
				setStyleAttr(e.id,"display",display);
			}
		}
	}
}
function hide(id) {
	var display = (arguments.length > 1) ? arguments[1] : "none";
	if (typeof(id) === "string" ) {
		var e = get(id);
		if (e != null) {
			setStyleAttr(id,"display",display);
		}
	} else {
		for (var i=0, j=id.length; i<j; i++) {
			var e = get(id[i]);
			if (e != null) {
				setStyleAttr(e.id,"display",display);
			}
		}
	}
}

/*** Part 6 ***/
function showInline(id) { show(id,"inline"); }
function hideInline(id) { hide(id,"hidden"); }
function displayInline(id) { show(id,"inline"); }
function hideInline(id) { hide(id,"hidden"); }
function toggle(id) {
	if(get(id)!=null)
	{
		switch(getStyleAttr(id,"display"))
		{
			case "block":
				hide(id);
				break;
			case "inline":
				hideInline(id);
				break;
			case "none":
				show(id);
				break;
			case "hidden":
				displayInline(id);
				break;
			default:
				show(id);
				hide(id);
				break;
		}
	}
}
function showDiv(id) { show(id); }
function hideDiv(id) { hide(id); }

/*** Part 7 ***/
function elementHide(id) { document.getElementById(id).style.visibility='hidden'; }
function elementShow(id) { document.getElementById(id).style.visibility='visible'; }

Part 1

The two first functions could be replaced by the following :

var get = document.getElementById;
var getByTagName = document.getElementsByTagName;

I would not recommend to use “get” as a function name, because it is a very common name that could be used for thousands of purposes.
Otherwise, my method would be probably lighter, because get and getByTagName are variables storing references. No new function needs to be stored in memory.

Part 2

While new Array(); is fine, writing [] does exactly the same thing and is shorter.

In the first loop, elements.length is evaluated at each iteration while we can see from the code that it cannot change. As a consequence, it could be cached for performance purpose. To go further, I would actually recommend to use .forEach instead of a loop, because it is quite clear that some “behavior” is expected “for each” element and “for each” class name.

Part 3

The first test (typeof(o) !== "undefined") is the only unambiguous way to test whether some variable is defined or not. As such, the second test (o!=undefined) is weird unless the intention is to test against an overridden value of undefined which I wouldn’t recommend. Testing against null is fine if it’s what is expected from the function. However the test against "null" seems to be somewhat arbitrary and I seriously hope that your name is not Null, because it might be the cause of a problem sometimes.

As far as I know, none of the if statement, the typeof operator or the comparisons can throw an error. As a consequence, wrapping in a try/catch is quite weird.

Part 4

A bunch of shorthand method. This is a matter of taste but I am not a big fan as long as it’s not to cover browsers irregularities as it could be with textContent (W3C) and innerText (IE-specific) which is not even the case.

formatURL don’t really need two instructions. More fundamentally, it does a redirection which is not obvious to understand from the name of the function.

Part 5

Code duplication is never a good idea. I would factorize. Moreover, a call to hide("someId", "block") doesn’t hide anything. As such, the name is not really good.

Part 6

function hideInline(id) { hide(id,"hidden"); } is simply written twice. That’s not actually the worse part. “hidden” is not a correct value for the display style property (the one filled by the hide function)

showInline and displayInline are the exact same function.

showDiv and hideDiv don’t act on divs exclusively. There is just no point for them to exist.

Part 7

In show and hide, the idea of checking if the result of document.getElementById(id) is null was a good one. The elementShow and elementHide functions may throw an error because of not doing this. Moreover, the fact of setting the “visibility” property instead of “display” this time and using the name function names (show/hide) is confusing.
A good factorization of show and hide could have parametrized the property and the default value, allowing to easily create functions to set “display” and “visibility” to their different (and respective !) values.

function stylePropertySetter(prop, defaultValue){
    return function(id){
                   var value = (arguments.length > 1) ? arguments[1] : defaultValue;
	           if (typeof(id) === "string" ) {
		       var e = get(id);
		       if (e != null) {
		           setStyleAttr(id, prop, value);
	               }
	           } else {
		        for (var i=0, j=id.length; i<j; i++) {
			      var e = get(id[i]);
			       if (e != null) {
				setStyleAttr(e.id, prop, value);
	    		     }
		   }
	    }
      };
}

var hide = stylePropertySetter("display", "none");

Conclusion

A lot of mistakes have been made. Some are just a matter of taste, some can crash your application, some may make the development phase trickier because of weird function names.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s