Tell me more ×
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: https://gist.github.com/4275805

any pointers in optimizing this much appreciated, for example the countries.forEach(function(c){ loop?

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
@khinester if the code does work, it should stay here. If it does not, it should be moved. (I think only admins can move it.) This is just my opinion, though, and JamesKhoury disagrees. – ANeves Dec 14 '12 at 15:07
show 2 more comments

Know someone who can answer? Share a link to this question via email, Google+, Twitter, or Facebook.

Your Answer

 
discard

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

Browse other questions tagged or ask your own question.