How can I simplify a long javascript switch case statement?

Question

I have the code below. It looks long and complicated and now I have to add some more conditions. Is there any way I can simplify this code. All I can think of is combining the City and Menu switches but then if I do that I have to do get the pk value one way for City and another way for Menu. Does javascript offer any other way to do the same thing as a switch? Sorry for the long code listing but I thought I should include everything.

function getParams(entity) {

 var store = window.localStorage;
 var table = "Content";

 switch (entity) {
    case "City":
        if (store.getItem('AccountID')) {
            pk = store.getItem('AccountID') + "04" + "000";
            return {
                pk: pk,
                param: '?pk=' + pk,
                table: table,
                rc: true
            }
        } else {
            paramOnFailure("Please reselect Account");
            return { rc: false }
        }
        break;
    case "Menu":
        if (store.getItem('AccountID')) {
            pk = store.getItem('AccountID') + "00" + "000";
            return { 
                pk: pk,
                param: '?pk=' + pk,
                table: table,
                rc: true
            }
        } else {
            paramOnFailure("Please reselect Account");
            return { rc: false }
        }
        break;
    case "Page":
        if (store.getItem('AccountID') && store.getItem('PageID')) {
            pk = store.getItem('AccountID') + store.getItem('PageID') + "000";
            return {
                pk: pk,
                param: '?pk=' + pk,
                table: table,
                rc: true
            }
        } else {
            paramOnFailure("Please reselect Account and Page Type");
            return { rc: false }
        }
        break;
    case "Question":
        if (store.getItem('AccountID') &&
            store.getItem('CityID') && 
            store.getItem('TopicID') ) {
            pk = store.getItem('AccountID') + store.getItem('CityID');
            return {
                pk: pk,
                param: '?pk=' + pk + '&rk=' + store.getItem('TopicID'),
                table: "Question",
                rc: true
            }
        } else {
            paramOnFailure("Please reselect Account, City and Topic");
            return { rc: false }
        }
        break;
    case "Reference":
        if (store.getItem('ReferenceID')) {
            pk = store.getItem('ReferenceID');
            return { 
                pk: pk,
                param: '?pk=' + pk,
                table: "Reference",
                rc: true
            }
        } else {
            paramOnFailure("Please reselect Reference");
            return { rc: false }
        }
        break;
    case "Topic":
        if (store.getItem('AccountID') && store.getItem('CityID')) {
            pk = store.getItem('AccountID') + "05" + store.getItem('CityID');
            return { 
                pk: pk,
                param: parameters = '?pk=' + pk,
                table: table,
                rc: true
            }
        } else {
            paramOnFailure("Please reselect Account and City");
            return { rc: false }
        }
        break;
    default:
        ;
}

Maybe it's better to ask this question at http://codereview.stackexchange.com/ — antyrat, Jul 04 '12 at 14:38
keep it DRY, you could easily write a function to do the blocks within each case statement — raddrick, Jul 04 '12 at 14:38
u deffo can check for store.getItem('AccountID') before switch — Elen, Jul 04 '12 at 14:38
Difficult since you are doing completely different things in all the cases... though your error hadling (paramOnFailure) could be moved - just set a 'failure' flag and handle that after the case — Charleh, Jul 04 '12 at 14:40

score 3 · Answer 1 · answered Jul 04 '12 at 14:39

3

If it were me I'd move the code from each case condition into their own functions. This should make things more legible.

answered Jul 04 '12 at 14:39

Lee Taylor

7,761
16
33
49

score 3 · Accepted Answer · edited Nov 27 '13 at 16:47

Put this part of you code in function and call it with the proper parameter

  if (store.getItem('AccountID')) {
             pk = store.getItem('AccountID') + "00" + "000";
             return {
                  pk: pk,
                 param: '?pk=' + pk,
                 table: table,
                 rc: true
             }
         } else {
             paramOnFailure("Please reselect Account");
             return { rc: false }
         }

So you code will be somthing like this and call this function from each Switch Case

function CommonforCase(store,value)
{
   if (store.getItem('AccountID')) {
                 pk = store.getItem('AccountID') + value;
                 return {
                      pk: pk,
                     param: '?pk=' + pk,
                     table: table,
                     rc: true
                 }
             } else {
                 paramOnFailure("Please reselect Account");
                 return { rc: false }
             } 

}

score 0 · Answer 3 · answered Jul 04 '12 at 14:43

Something like this:

function getTheItem(itemName, value, table){
         if (store.getItem(itemName)) {
            pk = store.getItem(itemName) + value;
            return {
                pk: pk,
                param: '?pk=' + pk,
                table: table,
                rc: true
            }
        } else {
            paramOnFailure("Please reselect"+ itemName);
            return { rc: false }
        }
}

score 0 · Answer 4 · edited May 23 '17 at 12:04

I did not scroll done at first, so I missed a lot of that switch statement (this might also be the case in some of the other answers). The first two cases are straightforward to factor out into a function. In the rest of your code, you have more differences, so you would need more arguments in that function, which can easily get confusing.

So instead, you could use a configuration object to have named options and use default values (for merging default and given values, see .extend() and merging objects dynamically). You could also build up the error message dynamically from all mandatory values. It should become shorter and more readable this way.

Some pseudo-code:

// Pseudo-Code
function getParams(entity) {
 var store = window.localStorage;
 var defaults = {
    "mandatoryValues":[],
    "pk":null,
    "table":"Content",
    "errorMessage":"Please reselect Account",
    "param":""
 };
 var foo = function(cfgObj) {
    // Add default values from defaults variable for missing properties in cfgObj
    // Set other missing values based on cfgObj (e.g. `param` from `pk`)
    // Return your object using the now 'complete' cfgObj
 }

 switch (entity) {
    case "City":
        var pk = store.getItem('AccountID') + "04" + "000";
        return foo(
            "mandatoryValues":[store.getItem('AccountID')],
            "pk":pk);
    // Menu, Page
    case "Question":
        var pk = store.getItem('AccountID') + store.getItem('CityID');
        return foo(
            "mandatoryValues":[
                store.getItem('AccountID'),
                store.getItem('CityID'),
                store.getItem('TopicID')],
            "pk":pk,
            "param":'?pk=' + pk + '&rk=' + store.getItem('TopicID'),
            "table":"Question");
    // Reference, Topic, default
}

How can I simplify a long javascript switch case statement?

4 Answers4