How to refactor a long else if statement?

Question

My code piece reached a cyclomatic limit, trying to think of a way to refactor.

if(item.oldLabelType === 'Fruits') {
  item.newLabel = this._processFruitsLabel(item.oldLabel);
}
else if(item.oldLabelType === 'Vegetables') {
  item.newLabel = this._processVegetablesLabel(item.oldLabel);
}
else if(item.oldLabelType === 'Animals') {
  item.newLabel = this._processAnimalsLabel(item.oldLabel);
}
else if(item.oldLabelType === 'Fish') {
  item.newLabel = this._processFishLabel(item.oldLabel);
}
else if(item.oldLabelType === 'Birds') {
  item.newLabel = this._processBirdsLabel(item.oldLabel);
}
else if(item.oldLabelType === 'Colors') {
  item.newLabel = this._processColorsLabel(item.oldLabel);
}
else if(item.oldLabelType === 'Countries') {
  item.newLabel = this._processCountriesLabel(item.oldLabel);
}
else if(item.oldLabelType === 'Drinks') {
  item.newLabel = this._processDrinksLabel(item.oldLabel);
}
else if(item.oldLabelType === 'Cars' || item.oldLabelType === 'Airplanes') {
  item.newLabel = this._processTransportationLabel(item.oldLabel);
}

Synopsis - I'm in the process of refactoring a code base, the back end returns undesirable values, i.e. old label for something might be "Only $1000", the new label needs to "You pay only $1000 today.". The label manipulation is radically different depending on the item.oldLabelType that's sent back. So I can't really write a one-size-fits-all function that will transform any and all of the old labels into new.

What to do!?

score 4 · Answer 1 · answered Aug 21 '17 at 16:36

4

The usual answers here are:

Use a switch (but it's not much of an improvement, or arguably an improvement at all)
Use a lookup Map or object
Use string concatenation and brackets notation

Here's how that third one would look:

var functionName = item.oldLabelType === 'Cars' || item.oldLabelType === 'Airplanes'
    ? "_processTransportationLabel"
    : "_process" + item.oldLabelType + "Label";
if (this[functionName]) {
    item.newLabel = this[functionName](item.oldLabel);
}

answered Aug 21 '17 at 16:36

T.J. Crowder

1,031,962
187
1,923
1,875

Third one for this case is beatiful (and, yet, the cleverest!), you deserve a +1 :). – briosheje Aug 21 '17 at 16:44
2

Personally, I'm not a fan of the third case, since it makes it hard for someone to search the code to find usages of the function. Doing a grep on, say, "_processColorsLabel" would not return any results and the function could be errantly deleted. – maxdeviant Aug 21 '17 at 16:48
@maxdeviant: And that's a very good point. I'd probably use Option 2 with the full names. – T.J. Crowder Aug 21 '17 at 16:59

maxdeviant · Answer 2 · 2017-08-21T22:00:55.023

Since functions are first-class citizens in JavaScript, we can do something like this:

var labelMapping = {
  Fruits: this._processFruitsLabel,
  Vegetables: this._processVegetablesLabel,
  Animals: this._processAnimalsLabel,
  Fish: this._processFishLabel,
  Birds: this._processBirdsLabel,
  Colors: this._processColorsLabel,
  Countries: this._processCountriesLabel,
  Drinks: this._processDrinksLabel,
  Cars: this._processTransportationLabel,
  Airplanes: this._processTransportationLabel
};

var processFn = labelMapping[item.oldLabelType];

if (typeof processFn === 'function') {
  item.newLabel = processFn(item.oldLabel);
} else {
  // Handle when we can't find the process function.
}

One caveat is that if you use this inside of the various process functions, then you'll need to make sure they get called with the right this context.

There are two ways of doing this:

.bind the functions ahead of time

Fruits: this._processFruitsLabel.bind(this),

Use .call and pass in the current this

item.newLabel = processFn.call(this, item.oldLabel);

score 1 · Answer 3 · answered Aug 21 '17 at 16:40

construc a object for map to the correct function at begining like this:

var targetFunctionMap = {
    "Fruits": this._processFruitsLabel,
    "Vegetables": this._processVegetablesLabel,
    "Animals": this._processAnimalsLabel
    .............
    .............
    .............
    "Cars": this._processTransportationLabel,
    "Airplanes": this._processTransportationLabel
}

and then invoke from there

item.newLabel = targetFunctionMap[item.oldLabelType].call(this);

zfrisch · Answer 4 · 2017-08-21T16:47:17.463

If most of your functions being called are simply reiterations of each other with a slight variation in name based on the label type, you can create the correct function name to call on the fly. In the event that any function name houses many label types you can use a switch statement to specify the groups function name.

if (item.oldLabelType) {
   let olt = item.oldLabelType;
   switch (olt) {
     case "Airplane":
     case "Car":
       olt = "Traffic"
       break;
   }
   let func = "_process" + olt + "Label";
   item.newLabel = this[func](item.oldLabel);
 }

How to refactor a long else if statement?

4 Answers4