current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 10 down vote favorite
1

I'm trying to learn how to write better, more efficient JavaScript code and wanted to run this function I'd written by you and see if people can suggest an improved method.

It's basically a people filter based upon two variables.

Demo.

It works fine, but if additional categories come in (so far there are three) then the code would become redundant.

var family = 'director';
var expertise = 'asset';


function showConsultant(){ 
   $('.profile').stop(true,true).fadeOut(100);
   $('.profile[data-family="'+family+'"][data-expertise="'+expertise+'"]').delay(101).fadeIn();
}

showConsultant();

$('.top-level li').on('click', function() {

  $('.top-level li').removeClass('selected');
  $(this).addClass('selected');

  if( $(this).index() === 0 ){
     family = 'director';
     showConsultant();
  } else if( $(this).index() === 1 ){
      family = 'consultant';
      showConsultant();
  } else if( $(this).index() === 2 ){
      family = 'support';
      showConsultant();
  }

});

$('.second-level li').on('click', function() {

  $('.second-level li').removeClass('selected');
  $(this).addClass('selected');

  if( $(this).index() === 0 ){
     expertise = 'asset';
     showConsultant();
  } else if( $(this).index() === 1 ){
      expertise = 'insurance';
     showConsultant();
  } else if( $(this).index() === 2 ){
      expertise = 'wealth';  
     showConsultant();
  }

});
share|improve this question
up vote 1 down vote accepted

I stripped out most of what seemed unnecessary to still make the code do the same thing. By adding an array and using the index as your reference you can eliminate the need for the if, then, else clauses.

var family = 'director';
var expertise = 'asset';

var familyArray = ['director', 'consultant', 'support'];
var expertiseArray = ['asset', 'insurance', 'wealth'];

function showConsultant(){ 
   $('.profile').stop(true,true).fadeOut(100);
   $('.profile[data-family="'+family+'"][data-expertise="'+expertise+'"]').delay(101).fadeIn();
}

showConsultant();

$('.top-level li').on('click', function() {

  $('.top-level li').removeClass('selected');
  $(this).addClass('selected');
  family = familyArray[$(this).index()];
  showConsultant();

});

$('.second-level li').on('click', function() {

  $('.second-level li').removeClass('selected');
  $(this).addClass('selected');
  expertise = expertiseArray[$(this).index()];
  showConsultant();

});

You could probably go one step further and write the function handler for the event separately such that the same function could handle both click events for the top-level and second-level li's. You'd just have to do a check for which class the li falls under (i.e. top-level or second-level) and change the appropriate variable (i.e. family or expertise). 

var family = 'director';
var expertise = 'asset';

var familyArray = ['director', 'consultant', 'support'];
var expertiseArray = ['asset', 'insurance', 'wealth'];

function showConsultant(){ 
   $('.profile').stop(true,true).fadeOut(100);
   $('.profile[data-family="'+family+'"][data-expertise="'+expertise+'"]').delay(101).fadeIn();
}

function clickHandler () {
  $(this).siblings().removeClass('selected');
  $(this).addClass('selected');
  $(this).parent().hasClass('top-level')
    ? family = familyArray[$(this).index()]
    : expertise = expertiseArray[$(this).index()];

  showConsultant();
}

showConsultant();

$('.top-level li').on('click', clickHandler);
$('.second-level li').on('click', clickHandler);
share|improve this answer
up vote 7 down vote

Make a lookup table. Then access by index:

var expertises = ["asset", "insurance", "wealth"];
expertise = expertises[$(this).index()];
showConsultant();

If you're worried about going out of bounds, add a range check:

var expertises = ["asset", "insurance", "wealth"];
var index = $(this).index();
if (0 <= index && index < expertises.length) {
    expertise = expertises[index];
    showConsultant();
}

You could even globally define these lists somewhere, so you could write enums.getExpertise(index).

share|improve this answer
up vote 2 down vote

The added comments explain my take on your code:

// Maybe rename this to something more intuitive
// This will allow you to add unlimited click combinations without additional logic
// This is the main piece which will help in avoiding redundancy
var list = [
    { // index 0
        family:'director',
        expertise:'asset'
    },
    { // index 1
        family:'consultant',
        expertise:'insurance'
    },
    { // index 2
        family:'support',
        expertise:'wealth'
    }
];

var family_index = 0;
var expertise_index = 0;

// Don't pollute the global space with functions
var showConsultant = function(){ 

    // Take advantage of the callback parameter of the fadeOut() function to guarantee the proper execution order
    // because .delay(101) might not fire off precisely after fadeOut() has finished
    $('.profile').stop(true,true).fadeOut(100, function(){

        // Call upon your list and the presently set indexes to figure out what to fadein()
        $('.profile[data-family="'+list[family_index].family+'"][data-expertise="'+list[expertise_index].expertise+'"]').fadeIn();
    });
};

showConsultant();

// Listen for clicks in either level and use .hasClass() to determine which level we are dealing with
$('.top-level li, .second-level li').on('click', function() {

    // $(this) is needed several times so just cache it
    var $this = $(this);
    var index = $this.index();
    var level = ($this.hasClass('top-level') ? 'top' : 'second');

    // Dynamically target the level
    $('.'+level+'-level li').removeClass('selected');
    $this.addClass('selected');

    // Make sure that what the user clicked is a defined choice in your list variable
    if( index < list.length ){

        if(level === 'top'){
            family_index = index;
        } else {
            expertise_index = index;
        }

        showConsultant();
    }

});
share|improve this answer
3  
You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. It might help if you pull the comments out of the code so they're easier to read. – Pimgd yesterday
    
Basically, you have a good answer, but I can't easily tell what you changed and why. – Pimgd yesterday
1  
Summarising you're code comments outside of the code block would make it much easier to identify your review aspects. At first glance, this looks a lot like a code dump. – forsvarir yesterday
2  
@Pimgd Sorry, this is my first time answering on this site but when I have some additional free time then I will look into breaking out the code and comments properly. Thank you – MonkeyZeus yesterday
    
@MonkeyZeus ping me once you've done so with a comment and I'll take another look at the answer; if it's good I'll vote it up =) – Pimgd yesterday

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.