Friday, February 8th, 2008
Refactoring your JavaScript for fun and profit
Category: JavaScript
He walks through a refactoring experience:
Let’s say the job was to add small link to every DIV in a document with the class collapsible that would show and hide the DIV. The first thing to do would be to use a library to get around the issues of cross-browser event handling. Let’s not concentrate on that for the moment but go for old school
oneventhandlers as we’re talking about different things here.
The code starts off as:
JAVASCRIPT:
-
-
collapser = function(){
-
var secs = document.getElementsByTagName('div');
-
for(var i=0;i<secs .length;i++){
-
if(secs[i].className.indexOf('collapsible')!==-1){
-
var p = document.createElement('p');
-
var a = document.createElement('a');
-
a.setAttribute('href','#');
-
a.onclick = function(){
-
var sec = this.parentNode.nextSibling;
-
if(sec.style.display === none'){
-
sec.style.display = 'block';
-
this.firstChild.nodeValue = 'collapse'
-
} else {
-
sec.style.display = 'none';
-
this.firstChild.nodeValue = 'expand'
-
}
-
return false;
-
};
-
a.appendChild(document.createTextNode('expand'));
-
p.appendChild(a);
-
secs[i].style.display = 'none';
-
secs[i].parentNode.insertBefore(p,secs[i]);
-
}
-
}
-
}();
and then he walks through some steps:
- Step 1: Remove look and feel
- Step 2: Remove obvious speed issues
- Step 3: Removing every label and name from the functional code
- Step 4: Use human-readable variable and method names
- Step 5: Comment, sign and possibly eliminate the last remaining clash with other scripts
-
-
// Collapse and expand section of the page with a certain class
-
// written by Christian Heilmann, 07/01/08
-
(function(){
-
-
// Configuration, change CSS class names and labels here
-
var config = {
-
indicatorClass : 'collapsible',
-
collapsedClass : 'collapsed',
-
collapseLabel : 'collapse',
-
expandLabel : 'expand'
-
}
-
-
var sections = document.getElementsByTagName('div');
-
for(var i=0,j=sections.length;i<j ;i++){
-
if(sections[i].className.indexOf(config.indicatorClass)!==-1){
-
sections[i].className += ' ' + config.collapsedClass;
-
var paragraph = document.createElement('p');
-
var triggerLink = document.createElement('a');
-
triggerLink.setAttribute('href','#');
-
triggerLink.onclick = toggleSection;
-
triggerLink.appendChild(document.createTextNode(config.expandLabel));
-
paragraph.appendChild(triggerLink);
-
sections[i].parentNode.insertBefore(paragraph,sections[i]);
-
}
-
}
-
function toggleSection(){
-
var section = this.parentNode.nextSibling;
-
if(section.className.indexOf(config.collapsedClass)!==-1){
-
section.className = section.className.replace(' ' + config.collapsedClass,'');
-
this.firstChild.nodeValue = config.collapseLabel
-
} else {
-
section.className += ' ' + config.collapsedClass;
-
this.firstChild.nodeValue = config.expandLabel
-
}
-
return false;
-
}
-
Which has you ending up with:
JAVASCRIPT:
Related Content:
4.2 rating from 12 votes
Step 0: check your strings are delimited correctly…
if(sec.style.display === none’){
:-P
I really don’t understand why should something that must be clicked be a anchor with “#” href. For that we have events that handles action.
Why not just plain span with onclick event ?
You can’t do the same styling on a span as you can on a link on older browsers.
@MaLi: I do it the same way, because links are usually already styled to look clickable ;)
Actually it is because links and buttons are accessible by keyboard and for screen readers, spans with click events are not. If there is a thing in HTML that does the job, use this, as it has the job for a reason.
And the element here should be a ‘button’ – if only styling those wasn’t such pain in the ass… But I agree that ‘a’ is much better then ‘span’ for example.