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)

• ANeves: 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.
• James Khoury: @ANeves yes it is. $cols is defined further up as var $cols = []; and $col is declared var $col; and then defined $col = $cols.pop();
• ANeves: @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?
• khinester: @ANeves shall i move this to stackoverflow, but i am unsure if it is possible?
• rolfl: ... but please, if/when it comes back, make sure the formatting is more readable (indenting, braces, etc... ) See @JamesKhoury answer.

Answer 1

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?

• James Khoury: @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?

Answer 2

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.