Software Engineering Stack Exchange is a question and answer site for professionals, academics, and students working within the systems development life cycle. It only takes a minute to sign up.
Sign up to join this community
I have a piece of code that composes method name to call from a string parameter. I don't feel it's a good thing to do but I'm not sure what can go wrong with this.
Here is a simplified snippet of that code:
switchToFilter = function(filter){
var filterMethod = 'switchTo' + filter;
myObject[filterMethod]();
}
At run time, the worst thing that could probably happen is that the method doesn't exist, so you get some sort of run-time error saying ERROR: method: 'switchToFoo' could not be found in context blah blah blah...
I'm not really sure if this type of code could be vulnerable to injection attacks. Where does the value for filter come from?
A bigger problem might be that whatever development tools you're using won't be able to work with this. For example, if you want your IDE to search for all places where switchToFoo is called, it won't be possible to find the ones called by the snippet you show above. Or if you want to refactor switchToFoo to switchToFou, you have to change the method name, and then change the code that passes the value for filter. You will know all about this, but this sort of thing usually trips up developers who are new to the project.
What can go wrong with composing a method name out of a string?
If myObject[filterMethod]() doesn't exist, you could crash your application.
If your application is part of the safety controls for nuclear missile fire control, you could be potentially disabling critical safety controls.
If other software for the controls doesn't account for this crash, you could allow anyone to fire ze missiles.
If someone is having a bad day, they could decide to try to launch the missiles and be surprised they launch.
If they launch the missiles, you could start a nuclear war.
If a nuclear war starts, you could end civilization as we know it.
FOR THE LOVE OF THE CHILDREN CHECK THINGS EXISTS BEFORE ACCESSING THEM!
The biggest risk is trying to access something that doesn't exist within the array structure. In the worst case, that will crash your application. In the best case, there wouldn't be any perceived operation going on.
If you wrapped that logic with some guard checks to make sure you weren't accessing something that wasn't there, it would be a little bit more robust. You could also consider adding logging for the errant requests that still manage to come in. With log information, you could hunt the problems down after the fact.
It's a form of reflection, which has its uses in dynamically-built applications (like a GUI builder for example) or plugins, but should be a matter of last resort outside those contexts.
In addition to the excellent reasons stated in other answers, it's a sign that your class hierarchy probably needs refactoring. Doing so will most likely make it much easier to reason about all your code. For one refactored example:
switchToFilter = function(filterName){
var filter = filters[filterName];
filter.switch();
}
This allows you to use inheritance to avoid duplication in the switch functions, and most likely has a side effect of providing a convenient place to put other filter-specific behavior.
filter is from an external source like a user you have a potential source of unintended errors, possibly a security issue depending on the contents of myobject.That being said I do use dispatch tables much like what you have in your example. They are useful in particular instances. The key is understanding what the costs (and risks) are and making sure they don't out weigh the benefits.
If the string comes from an insecure source (user input etc. ) it is absolutely, totally unacceptable. You voluntarily added a code injection vulnerability to your code.
You can write some code that will map a list of known function names to known functions, where you have verified yourself that each of those functions may be called without negative side effects.
