views:

842

answers:

4

I'm creating a contextual menu in javascript for a web app. The menu can appear in numerous contexts, and have different choices. I could have a different fuction for each context/choice:

grid1_delete()
grid1_duplicate()
grid2_delete()
grid2_add()
grid2_duplicate()

and hard code those in as the menu is being built. The thing I don't like about that is there will probably be a lot of repeated code. So I was thinking about using a dispatcher function, but it's resulting in potentially long, nested switch statement thus:

function contextMenuClick(context, menuItem) {
    var action = menuItem.innerHTML;
    switch (context) {
        case 'grid1':
            switch(action) {
                 case('delete'):
                      // do delete for grid1
                      break;
                 case('duplicate'):
                      // do duplicate for grid1
                      break;
                 default:
                      console.log('undefined action in contextMenuClick/grid1: ' + context);
            }
            break;
        case 'grid2':
            switch(action) {
                 case('add'):
                      // do add for grid2
                      break;
                 case('delete'):
                      // do delete for grid2
                      break;
                 case('duplicate'):
                      // do duplicate for grid2
                      break;
                 default:
                      console.log('undefined action in contextMenuClick/grid2: ' + context);
            }
            break;
        default:
            console.log('undefined context in contextMenuClick: ' + context);
        }

Yuck. There's got to be a better way. Maybe the dispatcher is more trouble than it's worth. I've looked at some of the related posts, but I'm not quite getting how to apply them to this exact situation.

+1  A: 

The easiest good simplification is to have each switch all a function containing a switch.

If you have an object for each of your contexts, you can add a function to each object which accepts the menu as an argument.

le dorfier
A: 

Something along the lines of using eval(context + "_" + action + "();") would likely work, but is quite dangerous, as it would allow your client to execute a nearly arbitrary function in your script. (anything that matches X_Y). Yes. eval is mostly evil.

How about

switch(context+"_"+action) { 
    case ("grid1_add"):  grid1_add(); 
[...]
}
slacy
+2  A: 

How about passing an actual object reference in for "context" instead of just a string? That way, you have just one switch statement:

function contextMenuClick(grid, menuItem) {
    var action = menuItem.innerHTML;
    switch(action) {
       case('delete'):
          grid.delete();
          break;
       case('duplicate'):
          grid.duplicate();
          break;
    }
}

Better yet, simply bind the handler directly to the correct object/method.

Chase Seibert
+2  A: 

Switch statements are very rarely necessary in Javascript. In general, you can just use objects like dictionaries/maps and do the lookup directly: foo.bar is equivalent to foo['bar'].

Also, for "global" variables, some_global_func() is equivalent to window.some_global_func(), which can also be written as var f = 'some_global_func'; window[f](): you don't ever need eval in order to select a variable or call a function dynamically based on its name. In general, when doing that, though, you should prefer to store the function in an object rather than at global scope (i.e. in the window object).

So, assuming that grid1_delete and grid2_delete are fundamentally different and can't be combined into a generic function, you could do something like the following without changing your code much:

var grid_actions = {
    'grid1': {
        'delete': function() { /* ... */ },
        'duplicate': function() { /* ... */ }
    },
    'grid2': {
        'delete': function() { /* ... */ },
        'add': function() { /* ... */ },
        'duplicate': function() { /* ... */ }
    }
}

function contextMenuClick(context, menuItem) {
    var action = menuItem.innerHtml;
    if (context in grid_actions) {
        if (action in grid_actions[context]) {
            grid_actions[context][action]();
        } else {
            console.log('undefined action in contextMenuClick/' + context + ': ' + action);
        }
    } else {
        console.log('undefined context in contextMenuClick: ' + context);
    }
}

A better solution, though, is to refactor things to have have these functions be methods of objects for each context, like @le dorfier suggests.

Miles