I have this code:

Tabzilla.fillZGContacts = function(){
   if (!Tabzilla.panel.id) return;
       url: 'zgcontacts.json',
      success: function(d){   // "Type","Name","Link","Contact","Location","Icon"
        Tabzilla.zgContacts = d;
        var countries = [];
          if (row[0] == 'Country') countries.push(
            {link:row[2], contact:row[3], country: row[4]}


        //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
          var firstletter = c.country.toLowerCase().charAt(0);
          if (byletter[firstletter]) byletter[firstletter]++;
          else byletter[firstletter]=1;
        //prepare containers
        var panel = $("#"+Tabzilla.panel.id);
        var $cols = []; 

        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.
          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();

               //does not work :(
               //could generate another template with first letter raised
          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:

  • \$\begingroup\$ 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. \$\endgroup\$
    – ANeves
    Dec 13, 2012 at 13:54
  • \$\begingroup\$ @ANeves yes it is. $cols is defined further up as var $cols = []; and $col is declared var $col; and then defined $col = $cols.pop(); \$\endgroup\$ Dec 13, 2012 at 23:30
  • \$\begingroup\$ @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? \$\endgroup\$
    – ANeves
    Dec 14, 2012 at 9:48
  • \$\begingroup\$ @ANeves shall i move this to stackoverflow, but i am unsure if it is possible? \$\endgroup\$
    – khinester
    Dec 14, 2012 at 13:38
  • 1
    \$\begingroup\$ ... but please, if/when it comes back, make sure the formatting is more readable (indenting, braces, etc... ) See @JamesKhoury answer. \$\endgroup\$
    – rolfl
    Jan 31, 2014 at 1:37

2 Answers 2


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

if (byletter[firstletter]){

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?

  • \$\begingroup\$ @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? \$\endgroup\$ Jan 31, 2014 at 2:05

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>' +
   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  = {};

      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.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.