CODEWITHSUNDEEP
javascriptjqueryrefactoring
londonjava
londonjava

Reputation: 13

How refactor switch in javascript for cleaner code?

I am trying to write some cleaner Javascript / jQuery code. How do I refactor this function into something cleaner and smaller. Definitely seems like there is a better way to do this. It works but i'm sure there is a better way of getting writing a function like this. Thanks in advance!

function filterForm(purpose, entry) {
    switch (purpose) {
        case 'Business' : 
            switch (entry) {
                case 'Single': 
                    $("#moreq1").css( "display", "block" );   
                    $("#moreq2").css( "display", "none"  );   
                    $("#moreq3").css( "display", "none"  );  

                    break;
                case 'Double': 
                    $("#moreq1").css( "display", "block" );   
                    $("#moreq2").css( "display", "none"  );   
                    $("#moreq3").css( "display", "none"  );  

                    break;
                case 'Multiple' : 
                    $("#moreq1").css( "display", "block" );   
                    $("#moreq2").css( "display", "block" );   
                    $("#moreq3").css( "display", "none"  );  

                    break;
                }

                break;
            case 'Private' : 
                switch (entry) {
                    case 'Single'   : 
                        $("#moreq1").css( "display", "block" );   
                        $("#moreq2").css( "display", "none"  );   
                        $("#moreq3").css( "display", "none"  );  

                        break;
                    case 'Double'   : 
                        $("#moreq1").css( "display", "none"  );   
                        $("#moreq2").css( "display", "none"  );   
                        $("#moreq3").css( "display", "none"  );  

                        break;
                    case 'Multiple' : 
                        $("#moreq1").css( "display", "none"  );   
                        $("#moreq2").css( "display", "none"  );   
                       $("#moreq3").css( "display", "none"  );  

                        break;
                }   

                break;
            case 'Tourist' : 
                switch (entry) {
                    case 'Single'   : 
                        $("#moreq1").css( "display", "block" );   
                        $("#moreq2").css( "display", "none"  );   
                        $("#moreq3").css( "display", "block"  );  

                        break;
                    case 'Double'   : 
                        $("#moreq1").css( "display", "none" );   
                        $("#moreq2").css( "display", "none"  );   
                        $("#moreq3").css( "display", "none"  );  

                        break;
                    case 'Multiple' : 
                        $("#moreq1").css( "display", "none"  );   
                        $("#moreq2").css( "display", "none" );   
                        $("#moreq3").css( "display", "none" );  

                        break;
                }       

                break;
            }
    }
}

Upvotes: 1

Views: 194

Answers (7)

Manoj Yadav
Manoj Yadav

Reputation: 6612

Try this:

function filterForm(purpose, entry) {
    var hideMap = {
        Business: {
            Single: [2, 3],
            Double: [2, 3],
            Multiple: [3]
        },
        Private: {
            Single: [2, 3],
            Double: [1, 2, 3],
            Multiple: [1, 2, 3]
        },
        Tourist: {
            Single: [2],
            Double: [1, 2, 3],
            Multiple: [1, 2, 3]
        }
    };

    hideShow(hideMap[purpose][entry]);
}

function hideShow(a) {
    var allEls = '#moreq1, #moreq2, #moreq3';
    for (var i = 0; i < a.length; i++) {
        a[i] = '#moreq' + a[i];
    }
    var hideEls = a.join(',');
    $(hideEls).hide(a.join());
    $(allEls).not(hideEls).show();
}

jsFiddle

Upvotes: 0

talemyn
talemyn

Reputation: 7960

As many people have stated, it looks like you could simplify the display logic considerably, but if it is important to you to maintain the different states based on the of the moreq1, moreq2, andmoreq3 elements, based on the purpose and entry variable values, then I would suggest that you shift the bulk of the display "logic" over to CSS, instead of JS/jQuery . . .

Most of that logic can be replaced with CSS style definitions that are tied to the purpose and entry values:

CSS

// "Business" display styles
// "Business - Single" display styles
.business.single #moreq1 {display: block}
.business.single #moreq2,
.business.single #moreq3 {display: none}

// "Business - Double" display styles
.business.double #moreq1 {display: block}
.business.double #moreq2,
.business.double #moreq3 {display: none}

// "Business - Multiple" display styles
.business.multiple #moreq1,
.business.multiple #moreq2 {display: block}
.business.multiple #moreq3 {display: none}

// "Private" display styles
// "Private - Single" display styles
.private.single #moreq1 {display: block}
.private.single #moreq2,
.private.single #moreq3 {display: none}

// "Private - Double" display styles
.private.double #moreq1,
.private.double #moreq2,
.private.double #moreq3 {display: none}

// "Private - Multiple" display styles
.private.multiple #moreq1,
.private.multiple #moreq2,
.private.multiple #moreq3 {display: none}

// "Tourist" display styles
// "Tourist - Single" display styles
.tourist.single #moreq1,
.tourist.single #moreq3 {display: block}
.tourist.single #moreq2 {display: none}

// "Tourist - Double" display styles
.tourist.double #moreq1,
.tourist.double #moreq2,
.tourist.double #moreq3 {display: none}

// "Tourist - Multiple" display styles
.tourist.multiple #moreq1,
.tourist.multiple #moreq2,
.tourist.multiple #moreq3 {display: none}

After that, you can simple assign styles using jQuery to the parent container (or even the <body> tag), using the purpose and entry values:

JS

function filterForm(purpose, entry) {
    var dynamicClasses = purpose + " " + entry;
    $("#parent_element_ID").addClass(dynamicClasses.toLowerCase());
}

At that point, the CSS will take over and show/display the elements based on your CSS rules.

While it may seem like a lot of CSS, not only does it use CSS for it's intended purpose, but it also makes any changes (or additions of new sections), a simple CSS update, rather than a JS code change.

Upvotes: 0

Bergi
Bergi

Reputation: 664936

I'd start with removing all hiding statements - just hide all in general and then show only those that need to (here with if-statements, see @nmynarcik's answer for an intermediate step with switches):

function filterForm(purpose, entry) {
    $("#moreq1, #moreq2, #moreq3").css("display", "none");

    if (purpose == 'Business') {
        $("#moreq1").css("display", "block");
        if (entry == 'Multiple')
            $("#moreq2").css("display", "block");
    } else if (purpose == 'Private' && entry == 'Single')
         $("#moreq1").css("display", "block");
    } else if (purpose == 'Tourist' && entry == 'Single') {
         $("#moreq1, #moreq3").css("display", "block");
    }
}

which could be further simplified to

function filterForm(purpose, entry) {
    $("#moreq1, #moreq2, #moreq3").css("display", "none");

    if (purpose == 'Business' || entry == 'Single')
         $("#moreq1").css("display", "block");
    if (purpose == 'Business' && entry == 'Multiple')
         $("#moreq2").css("display", "block");
    if (purpose == 'Tourist' && entry == 'Single')
         $("#moreq3").css("display", "block");
}

If it's getting more complicated I'd recommend a lookup map for the single display values, instead of using a switch statement with much repeated statements. For your setup it would look like

var show = {
    "moreq1": {
        "Business": true,
        "Private": {
            "Single": true
        },
        "Tourist": {
            "Single": true
        }
    },
    "moreq2": {
        "Business": {
            "Multiple": true
        }
    },
    "moreq3": {
        "Tourist": {
             "Single": true
        }
    }
};
for (var id in show) {
    var l = show[id];
    $("#"+id).css("display", l[purpose] === true || l[purpose] && l[purpose][entry] ? "block" : "none");
}

Of course you could swap the order of purpose and entry if that gets shorter.

Upvotes: 1

Ian
Ian

Reputation: 50913

While the other answers have done a decent job of combining the possibilities, it seems more logical to me to use a "map" of all the possibilities, and reference that:

var filterForm = (function () {
    var moreq = [
        "",        // So you can use indexes starting at 1
        "moreq1",
        "moreq2",
        "moreq3"
    ], Purposes = {
        "Business": {
            "Single": [moreq[1]],
            "Double": [moreq[1]],
            "Multiple": [moreq[1], moreq[2]]
        },
        "Private": {
            "Single": [moreq[1]],
            "Double": [],
            "Multiple": []
        },
        "Tourist": {
            "Single": [moreq[1], moreq[],
            "Double": [],
            "Multiple": []
        }
    };

    return function (purpose, entry) {
        var i, j, cur, display;

        for (i = 1, j = moreq.length; i < j; i++) {
            cur = moreq[i];
            display = Purposes[purpose][entry].indexOf(cur) > -1 ? "block" : "none";
            console.log("Setting " + cur + " as " + display);
            //$("#" + cur).css("display", display);
        }
    };
}());

filterForm("Business", "Multiple");

DEMO: http://jsfiddle.net/w3Jnn/1/

The arrays in the object are which elements to set the display as block. As soon as you want to change a specific setting, just modify the Purposes object's arrays.

And of course, my function doesn't do much checking the Purposes[purpose][entry].indexOf(cur) > -1 part - it assumes the purpose and entry parameters will have valid values. This could be modified to check, but it didn't seem important right now.

As a note, this uses a closure (the surrounding (function () { })) to instantiate the Purposes and items variables once, yet keep them out of the global scope.

Also note, the Array.indexOf method isn't supported in all browsers (mainly old IE), so there's a shim at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf#Compatibility to include on your page to make sure it works, among other ones you can find with Google.

Upvotes: 1

Jonathan
Jonathan

Reputation: 267

I'm not sure how much I like this way but this is a non-switch, non-if way of doing it. It can also help for testing as well since you could unit test lookup independently.

However, if purpose or entry aren't keys in lookup then you'll get some errors thrown.

function filterForm(purpose, entry) {
    var lookup = {}
    lookup['Business'] = {'Single':{'q1':'block','q2':'none','q3':'none'},
                         {'Double':{'q1':'block','q2':'none','q3':'none'}};

    lookup['Private'] = {...etc...};

    $('#moreq1').css('display', lookup[purpose][entry]['q1]);
}

Upvotes: 1

iamredseal
iamredseal

Reputation: 41

If you want to stick to using switched, give them all a class and you can set all to display: none; when you start your method.

Then doing:

function filterForm(purpose, entry) {

   $('.qItem').hide();

    switch (purpose) {

        case 'Business' : 

            $("#moreq1").css( "display", "block" );

            switch (entry) {

                case 'Multiple' : 

                    $("#moreq2").css( "display", "block" );

                break;

            }

        break;

        case 'Private' : 

            switch (entry) {

                case 'Single'   : 

                    $("#moreq1").css( "display", "block" );   
                    break;

            }   

        break;

        case 'Tourist' : 

            switch (entry) {

                case 'Single'   : 

                    $("#moreq1").css( "display", "block" );   
                    $("#moreq3").css( "display", "block"  );  

                break;

            }       

        break;

    }

}

You can see here how the class is used: http://jsfiddle.net/pMfgy/

Upvotes: 0

bfavaretto
bfavaretto

Reputation: 71939

After looking at all your conditions, it seems to boil down to this:

function filterForm(purpose, entry) {
    var q1 = purpose == 'Business' || entry == 'Single' ? 'block' : 'none';
    var q2 = purpose == 'Business' && entry == 'Multiple' ? 'block' : 'none';
    var q3 = purpose == 'Tourist' && entry == 'Single' ? 'block' : 'none';

    $("#moreq1").css( "display", q1 );   
    $("#moreq2").css( "display", q2 );   
    $("#moreq3").css( "display", q3 ); 
}

Upvotes: 1

Related Questions