Reputation: 14950
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
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
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
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