1
\$\begingroup\$

The application I am developing with has many input fields that interact with one another via the onchange event. When developing started, the goal was to create a framework that makes it easy to define event actions for the input fields and keep the action functions together.

The back then the following was implemented:

<input type='text' class='process' data-fn='function1;function2'>

And in Javascript using jQuery (leaving out security and validation checks here):

$("body").on("change", ".process", function(e){
     process($(this).attr("data-fn"),$(this),e);
});

function process(functionString, caller, event){
    var functions = functionString.split(";");
    for (var index = 0; index < functions.length; ++index) {
        switch (functions[index]){
            case "function1":
                    //do something
               break;
            case "function2":
                    //do something
               break;
        }
    }
}

The amount of needed functions was highly underestimated. The switch now has 110 cases. The advantage of the algorithm as it is now is that I can make the break optional, creating functions with multiple "entrances". The disadvantage is no local variable scope inside each case.


Is there a huge problem that I don't see with this approach (like switch performance etc.)?

I am interested if this is bad coding or just an interesting approach to the problem.

Additionally, does a software design pattern exist, that is defined to do such functionality?


I guess a better approach would have been the following, but now that it is that way..

function process(functionString, caller, event){
    var functions = functionString.split(";");
    for (var index = 0; index < functions.length; ++index) {
        if (functionObj.hasOwnProperty(functions[index]){
            functionObj[functions[index]](caller,event);
        }
    }
}

var functionObj = {
    function1: function (caller, event){
        //do something;
    },
    function2: function (caller, event){
        //do something;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Being a pedant: if you're looking for a production-ready solution instead of rolling your own, try looking at rivets. \$\endgroup\$ – Dan Pantry Oct 23 '15 at 10:51
  • \$\begingroup\$ @DanPantry This system is already in production.. I am now optimizing mistakes of the past. There are many things I would do different now, the biggest mistake was to implement own solutions for tasks where really good frameworks already had existed. But changing such basic procedures in hindsight takes a near impossible effort. \$\endgroup\$ – dboth Oct 23 '15 at 11:10
1
\$\begingroup\$

This sort of reminds me of how React implements Flux, where there's a huge switch list, and it becomes very unwieldy fast. Another developer created the Reflux library using the same concept in your alternative, a huge action map. So you're getting there. Let's just tweak your approach a bit.

First is you can use the noop approach, so you avoid the if and the function is always a function. It's pretty functional programming, but it streamlines your code, avoiding branching, unnecessary assignments and all that stuff.

Next, functionObj could be named better, like registeredFunctions. Also create an interface for you to add easily, as well as check for duplicates to prevent collision, or just register multiple ones under the same name.

Next is to allow your callers to define multiple args by not limiting the args. Here, we pass in arguments using apply. That way, the callers can provide any number of args they want and the receiving functions can still receive them. Of course, we slice off the first argument, which is your string of functions. You can assign any value as context for your registered function, but I prefer functions like these to have a context of null. Context becomes confusing when calling functions dynamically, and without a set convention.

var noop = function(){}
var registeredFunctions = {/* map of functions */}

function process(){
  var args = Array.prototype.slice.call(arguments, 1);
  functionString.split(";").forEach(function(functionName){
    (registeredFunctions[functionName] || noop).apply(null, args);
  });
}

An alternative would be to keep the huge switch, but instead of putting the actual code inside it, you call a function. There, you have a new scope. You just need to pass in everything you need into the function.

var args = Array.prototype.slice.call(arguments, 1);
switch(something){
  case 1:
  case 2:
    doSomething.apply(null, args);
    break;
  case 3:
    doSomethingElse.apply(null, args);
  default:
    doSomethingDefault.apply(null, args);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.