Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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)
share|improve this question
    
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 Dec 13 '12 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 Dec 13 '12 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 Dec 14 '12 at 9:48
    
@ANeves shall i move this to stackoverflow, but i am unsure if it is possible? –  khinester Dec 14 '12 at 13:38
1  
... but please, if/when it comes back, make sure the formatting is more readable (indenting, braces, etc... ) See @JamesKhoury answer. –  rolfl Jan 31 at 1:37

2 Answers 2

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?

share|improve this answer
1  
+1, though I do not agree with your dislike of forEach(). –  konijn Jan 31 at 1:38
    
@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 Jan 31 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>' +
                  '</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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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