newbie
newbie

Reputation: 14950

How to do the Jquery if-else statement better

Is there a way on how I can improve this if else statement below:

function updateStatusCount(){
    var openCount = 0;
    var ongoingCount = 0;
    var atRiskCount = 0;
    var missedCount = 0;
    var closedCount = 0;
    var onHoldCount = 0;
    $('.sr-header-status').each(function(){
        var value = $(this).html().toLowerCase();
        if(value === "open"){
            openCount = openCount + 1;
        } else if (value === "ongoing"){
            ongoingCount = ongoingCount + 1;
        } else if (value === "at risk"){
            atRiskCount = atRiskCount + 1;
        } else if (value === "missed target"){
            missedCount = missedCount + 1;
        } else if (value === "closed"){
            closedCount = closedCount + 1;
        } else if (value === "on hold"){
            onHoldCount = onHoldCount + 1;
        }
    });
    $('.statusCount').empty();
    $('.open.status_filter').find('span.statusCount').html(openCount);
    $('.ongoing.status_filter').find('span.statusCount').html(ongoingCount);
    $('.atrisk.status_filter').find('span.statusCount').html(atRiskCount);
    $('.missedtarget.status_filter').find('span.statusCount').html(missedCount);
    $('.closed.status_filter').find('span.statusCount').html(closedCount);
    $('.onhold.status_filter').find('span.statusCount').html(onHoldCount);
}

I'm thinking something like this

function updateStatusCount(){
    getStatusCount("open");
    getStatusCount("ongoing");
    getStatusCount("on hold");
    getStatusCount("at risk");
    getStatusCount("missed target");
    getStatusCount("closed");
}

function getStatusCount(status){
    var count = 0;
    var statusClass = "."+status.replace(/ /g,'');
    var $statusContainer = $(statusClass).find('span.statusCount');
    $('.sr-header-status').each(function(){
        var value = $(this).html().toLowerCase();
        if(value === status){
            count = count + 1;
        }
    });
    $statusContainer.empty();
    $statusContainer.html(count);
}

but which is better?

Thank you

Upvotes: 0

Views: 134

Answers (3)

Jose Rui Santos
Jose Rui Santos

Reputation: 15319

If you have a finite use cases, then using a switch statement seems to create more simple and legible code. I have made some code refactoring also.

function updateStatusCount(){
    var openCount = ongoingCount = atRiskCount = missedCount =
        closedCount = onHoldCount = 0;

    $('.sr-header-status').each(function(){
        switch ($(this).html().toLowerCase()) {
          case "open":          ++openCount;    break;
          case "ongoing":       ++ongoingCount; break;
          case "at risk":       ++atRiskCount;  break;
          case "missed target": ++missedCount;  break;
          case "closed":        ++closedCount;  break;
          case "on hold":       ++onHoldCount;
        }
    });
    $('.statusCount').empty(); // only need this line if there is other .statusCount not defined below
    $('.open.status_filter span.statusCount').html(openCount);
    $('.ongoing.status_filter span.statusCount').html(ongoingCount);
    $('.atrisk.status_filter span.statusCount').html(atRiskCount);
    $('.missedtarget.status_filter span.statusCount').html(missedCount);
    $('.closed.status_filter span.statusCount').html(closedCount);
    $('.onhold.status_filter span.statusCount').html(onHoldCount);
}

Solution 2 In jQuery 1.8+ you can define a new selector:

$.expr[":"].containsNoCase = $.expr.createPseudo(function(arg) {
    return function( elem ) {
        return $(elem).text().toLowerCase().indexOf(arg.toLowerCase()) >= 0;
    };
});

or this one, if your jquery is older:

$.expr[':'].containsNoCase  = function(a, i, m) { 
  return $(a).text().toLowerCase().indexOf(m[3].toLowerCase()) >= 0; 
};

and finally your function can be simplified to

function getStatusCount(status){
    var statusClass = "." + status.replace(/ /g,''),
        $statusContainer = $(statusClass).find('span.statusCount');
    $statusContainer.text($('.sr-header-status:containsNoCase("' + status + '")').length);
}

Upvotes: 1

jfriend00
jfriend00

Reputation: 707656

You can use an object as a lookup mechanism and let the object lookup handle all the comparisons for you rather than coding each one individuallly:

function updateStatusCount(){
    var counts = {
        "open": 0, "ongoing": 0, "at risk": 0,
        "missed target": 0, "closed": 0, "on hold": 0
    }
    $('.sr-header-status').each(function(){
        counts[$(this).html().toLowerCase()]++; 
    });
    $('.statusCount').empty();
    $('.open.status_filter').find('span.statusCount').html(counts["open"]);
    $('.ongoing.status_filter').find('span.statusCount').html(counts["ongoing"]);
    $('.atrisk.status_filter').find('span.statusCount').html(counts["at risk"]);
    $('.missedtarget.status_filter').find('span.statusCount').html(counts["missed target"]);
    $('.closed.status_filter').find('span.statusCount').html(counts["closed"]);
    $('.onhold.status_filter').find('span.statusCount').html(counts["on hold"]);
}

Or, you could get even a little more clever by generating the class name you're looking for:

function updateStatusCount(){
    var counts = {
        "open": 0, "ongoing": 0, "at risk": 0,
        "missed target": 0, "closed": 0, "on hold": 0
    }
    $('.sr-header-status').each(function() {
        counts[$(this).html().toLowerCase()]++; 
    });
    $('.statusCount').empty();
    for (var prop in counts) {
        $("." + prop.replace(/\s/g, "") + ".status_filter" + " span.statusCount").html(counts[prop]);
    }
}

Upvotes: 4

Milind Anantwar
Milind Anantwar

Reputation: 82241

use switch instead:

   $('.sr-header-status').each(function(){
    var value = $(this).html().toLowerCase();

switch(value){
case "open":
    openCount = openCount + 1;
break;

case "at risk":
    atRiskCount = atRiskCount + 1;
break;

case "missed target":
   missedCount = missedCount + 1;
break;
//and so on
.......
}});

Upvotes: 0

Related Questions