Removing the Repetition in Code (The Array)

Introduction

After spending some time tidying up a predecessor's javascript code I feel the need to document my findings. Any good developer will know all of this stuff, but while I'll go through explaining how to reduce the number of lines in your code, it's worth bearing in mind that we need to balance this with the idea of avoiding obfuscation.  

Obfuscation is the act of creating code that is difficult for humans to understand. Wikipedia suggests this is done deliberately, but I think that sometimes the best intentions go awry.

The Code as I found it

A function had been written to hide a large collection of bitmaps used to indicate validation failures on a web form.

function showimg(imgId,imgAct) {
    a = document.getElementById(imgId);
    if (imgAct == 'show' ) {
        a.style.display='block';
    } else {
        a.style.display='none';
    }
}

This was called in the main processing block in the following way:-

showimg('imgoriginator','hide');
showimg('imgeffectdate','hide');


//....loads more lines like this!!

showimg('entothimg','hide');
showimg('eooimg','hide');


Now if we ignore the fact that the showimg() function isn't very robust (it will generate an error if an object name isn't found in the DOM), it seems like an OK way of doing things.

The problem I had was that there were 124 of these calls, probably the result of a copy-&-paste frenzy. And while there's nothing actually wrong with doing this, it makes the code difficult to maintain. (and annoys people like me)

Let's say at some stage that we wanted to modify the parameters passed to the showimg() function. All of a sudden we have over a hundred lines to change. It might have started out simple, but now it's a complete pain!

How do we make it better?

The answer is to use an array to hold the image IDs, and then use a loop to process them.

I'd only use it if we get more than a handful of calls because the added complexity would hinder more than it helped. But, with 124 lines it's well worth bother, so first of all we'll define an array and fill it using the data from the original calls:

var imgArr = ['imgoriginator','imgeffectdate','imggroupnumber','imggroupname','imgaccountnumber','imgaccountnumber1','imgaccountnumber2',  'imgaccountnumber3','imgaccountnumber4','imgaccountnumber5','imgbranchname',
'imgtradingname','imgrequestforchange','imgreasonforchange','imgjustrequest','imgaccountmarker','imgremoverrider',
'discat1','overdis1','disthre1','saltarcat1','saltar1','payacc1','payto1','addl1','city1','county1','postcode1','justod1',
'discat2','overdis2','disthre2','saltarcat2','saltar2','payacc2','payto2','addl2','city2','county2','postcode2','justod2',
'discat3','overdis3','disthre3','saltarcat3','saltar3','payacc3','payto3','addl3','city3','county3','postcode3','justod3',
'discat4','overdis4','disthre4','saltarcat4','saltar4','payacc4','payto4','addl4','city4','county4','postcode4','justod4',
'discat5','overdis5','disthre5','saltarcat5','saltar5','payacc5','payto5','addl5','city5','county5','postcode5','justod5',
'discat6','overdis6','disthre6','saltarcat6','saltar6','payacc6','payto6','addl6','city6','county6','postcode6','justod6',
'discat7','overdis7','disthre7','saltarcat7','saltar7','payacc7','payto7','addl7','city7','county7','postcode7','justod7',
'th1','th2','th3','tl1','pt1','pt2','pt3','pl1','qeimg','gskimg','nqeimg','piimg','genimg','zdimg','shimg','pmedimg',
'gslmedimg','hbimg','otherimg','totalaahimg','trigenimg','triplsimg','entothimg','eooimg'];


Then we have a simple 'For' loop:

for (var i = 0; i < imgArr.length; i++){
   showimg(imgArr[i],'hide');
}

... and that's it!

If you need to add further field IDs it's a simple task of adding them to the array definition. There's no need to worry about getting the calls right, they just do the same as all the others.

An Alternative

Some of you will probably have your own ideas about how to improve this code.

One method to remove the array definition altogether requires you modify the web page to set each of the image class names to 'validate',... yes all 124 of them! And then have code that builds the array directly from the DOM..

var imgArr = document.getElementsByClassName('validate');

The loop needs a slight tweak because the array is now an array of tag objects, or we could do away with the showimg() function call and perform the hide within the loop..

for (var i = 0; i < imgArr.length; i++){
   imgArr[i].style.display='none';
}

Can't get much simpler than that, and maintenance is easier still, but it's now not so clear how it works! 


I'll leave you with a joke that Microsoft recently tweeted...

Q: Why did the programmer quit his job?

A: Because he didn't get arrays!