Optimise JavaScript loop

Question

I have this code:

Tabzilla.fillZGContacts = function(){ if (!Tabzilla.panel.id) return; $.ajax({ url: 'zgcontacts.json', success: function(d){ // "Type","Name","Link","Contact","Location","Icon" Tabzilla.zgContacts = d; var countries = []; d.rows.forEach(function(row){ if (row[0] == 'Country') countries.push( {link:row[2], contact:row[3], country: row[4]} ); }); //alphabetically countries.sort(sortByKey('country')); //adding link countryTemplate = function (country){ s = '<a title="'+country.country+'" class="chapters_link" href="' +country.link+'" target="_blank">' +'<div class="chapters c_'+country.country.toLowerCase()+'">' +'<span class="flag-margin">'+country.country+'</span></div></a>' return s; } var byletter = {}; //count countries starting from each letter countries.forEach(function(c){ var firstletter = c.country.toLowerCase().charAt(0); if (byletter[firstletter]) byletter[firstletter]++; else byletter[firstletter]=1; }); console.log(byletter); //prepare containers var panel = $("#"+Tabzilla.panel.id); var $cols = []; $cols.push(panel.find(".c_COL4")); $cols.push(panel.find(".c_COL3")); $cols.push(panel.find(".c_COL2")); $cols.push(panel.find(".c_COL1")); var columns = $cols.length; var targetlen = countries.length/columns; var FirstLetter = countries[0].country.toLowerCase().charAt(0); var cc = []; //fill containers. this loop is buggy. should be reviewed. countries.forEach(function(c){ var newFirstLetter = c.country.toLowerCase().charAt(0); if (FirstLetter != newFirstLetter) { var l1 = cc.length; var l2 = l1 + byletter[newFirstLetter]; //condition maybe shd be changed.. if (Math.abs(l2-targetlen) >= Math.abs(l1-targetlen)){ var $col; if ($cols.length>0) $col = $cols.pop(); cc.forEach(function(c){ $col.append(countryTemplate(c)); }); cc=[]; //does not work :( //could generate another template with first letter raised $col.find('span').first().addClass("first-letter"); } cc.push(c); } else cc.push(c); FirstLetter = newFirstLetter; }); }, }); }

The zgcontacts.json file can be found here.

Any pointers in optimizing this is much appreciated. For example, this loop:

countries.forEach(function(c)

Does not belong here, because the code does not work. var $col; if ($cols.length>0) $col = $cols.pop(); cc.forEach(function(c){ $col.append(countryTemplate(c)); }); This part cannot work, $col is undefined. — ANeves, CommentedDec 13, 2012 at 13:54
@ANeves yes it is. $cols is defined further up as var $cols = []; and $col is declared var $col; and then defined $col = $cols.pop(); — James Khoury, CommentedDec 13, 2012 at 23:30
@JamesKhoury thanks for correcting. But (1.) if cols.length == 0? $col is undefined, and one cannot do $col.append; unless this never happens, in which case the if is vestigial code and only serves to trick or confuse the reader. Also, (2.) what about that //does not work :( comment in the code? — ANeves, CommentedDec 14, 2012 at 9:48
@ANeves shall i move this to stackoverflow, but i am unsure if it is possible? — khinester, CommentedDec 14, 2012 at 13:38
... but please, if/when it comes back, make sure the formatting is more readable (indenting, braces, etc... ) See @JamesKhoury answer. — rolfl, CommentedJan 31, 2014 at 1:37

James Khoury · Accepted Answer · 2014-01-31 00:40:47Z

Use braces { in all your if blocks. It makes it easier to read.

if (byletter[firstletter]){ byletter[firstletter]++; }else{ byletter[firstletter]=1; }

You have for loops like : countries.forEach(function(c){ which are also hard to read. I'd only use this if you have a function you want to apply.

for(var countryindex = 0; countryindex < countries.length; countryindex++) { var c = countries[countryindex]; // ... }

Use descriptive variable names.

if (FirstLetter != newFirstLetter) { var l1 = cc.length; var l2 = l1 + byletter[newFirstLetter];

What is l1 & l2 ? What is c or cc?

@konijn javascript has for and for .. in I think they are much more readable and debuggable than [].forEach(. When would you favour it over the more traditional for loops? — James Khoury, CommentedJan 31, 2014 at 2:05

konijn · Accepted Answer · 2014-01-31 01:41:42Z

The location of your countryTemplate function breaks the flow of your code, you should put it at the very end. Also I would encourage you to use some templating routine, you could use this one:

function fillTemplate( s ) { //Replace ~ in s with further provided arguments for( var i = 1 ; i < arguments.length ; i++ ) s = s.replace( "~" , arguments[i] ); return s; }

Then your countryTemplate could be :

 countryTemplate = function (country) { var template = '<a title="~" class="chapters_link" href="~" target="_blank">' + '<div class="chapters c_~">' + '<span class="flag-margin">~</span>' + '</div>' + '</a>'; return fillTemplate( template , country.country , country.link , country.country.toLowerCase() , country.country }

This could be DRY'er of course, since I repeat country.country a number of times, I leave the final code to you.

Furthermore, there is no reason not to merge the extraction of the countries and the calculation of byletter, it will save you a 'forEach` call:

 var countries = [], byletter = {}; d.rows.forEach(function(row){ if (row[0] == 'Country'){ var country = { link:row[2], contact:row[3], country: row[4] }; var firstletter = row[4].toLowerCase().charAt(0); countries.push( country ); byletter[firstletter] = ( byletter[firstletter] || 0 ) + 1; } });

Finally, your buggy loop is very badly written, I cannot tell what you are trying to achieve with that code. Maybe you should comment each section with what it is supposed to do and indeed follow the suggestions of James Khoury.

Stack Exchange Network

Optimise JavaScript loop

2 Answers 2

Hot Network Questions

Optimise JavaScript loop

2 Answers 2

Related

Hot Network Questions