1

I have one function which is having if elseif conditions and the cyclomatic complexity is approaching 5. How do I reduce it?

function testFunc() {
    var step = getModel('step');
    if(step === 1) {
        this.resetTask(); //calling some function
        this.updateStep(0);
        return true;
    } else if(step === 2) {
        this.initTask; //some other function
        return true;
    } else if(step === 3) {
        this.name === 'add' ? this.add() : this.edit();
        return true;
    }
    return false;
}

tried replacing with switch case but it didn't help.

mplungjan
  • 169,008
  • 28
  • 173
  • 236
Liz.
  • 795
  • 2
  • 13
  • 31
  • @Liz.: Without knowing what your code is actually doing, it’s hard to say. Your goal is to reduce the number of paths through the function (or decide that the complexity is unavoidable and tell the linter to go away), and doing that appropriately means thinking about the whole system. If this is real code, I might start at `this.name === 'add' ? this.add() : this.edit();`, though. It seems off. But maybe the whole step system has a better alternative, like putting each step into a separate function, or using one coroutine. Or maybe you should exclude state machinery from cyclomatic complexity. – Ry- Apr 09 '20 at 11:12
  • mainly I have return boolean value for each scenario but also call some function. You can consider it as a step form where user is on form step 1 and if he navigates back the form must reset and step is updated as 0. Then for 2nd step, if user goes back he is navigated to first form – Liz. Apr 09 '20 at 11:16
  • @ry this is real function logic. the last condition is step 3 can exist in both add and edit form . So based on the name I have to perform the add/edit task – Liz. Apr 09 '20 at 11:18

3 Answers3

3

Very simple refactor - remove all conditional logic you have now and extract each piece as a separate function into a map. Since you only execute one branch each time, depending on step, you can make that value the key and fetch what needs to be executed. You can then provide a fallback for when there is nothing that corresponds to step.

Now the cyclomatic complexity is 2, since there is only one place the code branches - either you find the corresponding handler for step or not. Also, the branching in step 3 is a completely separate function now, thus it doesn't need to count as part of testFunc

function testFunc() {
  var step = getModel('step');

  var steps = {
    1: function() {
      this.editTask(); //calling some function
      this.updateStep(0);
      return true;
    },
    2: function() {
      this.initTask; //some other function
      return true;
    },
    3: function() {
      this.name === 'add' ? this.add() : this.edit();
      return true;
    },
    default: function() { 
      return false; 
    }
  };

  var fn = steps[step] || steps.default;

  return fn();
}
VLAZ
  • 26,331
  • 9
  • 49
  • 67
  • this looks readable and it reduced the complexity as well. Thanks :) – Liz. Apr 09 '20 at 12:01
  • 1
    @Liz. I didn't mention it explicitly but you can also take `steps` out of the function, so `testFunc` would be even easier to read. It's also good practice, since you now have the logic for the steps completely decoupled from how they are used - you can add/edit/remove steps without touching `testFunc`. – VLAZ Apr 09 '20 at 12:06
  • yes its a good practice. But for my code block it is going to be same therefore I have kept inside itself. I learnt something new today. thank you – Liz. Apr 09 '20 at 12:18
  • i added one default condition where I am returning false. so its more readable. so my last condition is like : `steps[currentStep] || steps.default` – Liz. Apr 09 '20 at 12:20
  • @Liz. yes, that's a good idea, actually - explicitly name the default, so it's clear what you're falling back to. – VLAZ Apr 09 '20 at 12:21
  • 1
    you can edit the answer with default. for others help – Liz. Apr 09 '20 at 12:26
  • 1
    @Liz. I use [JSHint's demo](https://jshint.com/) - it provides you with cyclomatic complexity in the Metrics section. Copy/paste your code in the right pane and you'll see it to the left. – VLAZ Apr 09 '20 at 13:43
3

This will reduce your code complexity and makes more readable. You can decouple your condition into multiple function and attach that function in condition object. here I'm passing this in argument but you can use different approch. here I've also given value of this just for the shake of running example. copy and paste in your editor and try.

function meaningfulFuncName1() {
  this.editTask(); //calling some function
  this.updateStep(0);
  return true;
}

function meaningfulFuncName2() {
  this.initTask; //some other function
  return true;
}

function meaningfulFuncName3(context) {
  context.name === 'add' ? context.add() : context.edit();
  return true;
}

function defaultCase() {
  return false;
}

function testFunc() {
  this.name = 'add'
  const step = getModel('step')
  conditionObject = {
    1: meaningfulFuncName1,
    2: meaningfulFuncName2,
    3: meaningfulFuncName3,
    default: defaultCase
  }
  return conditionObject[step](this);
}
Shubham
  • 756
  • 4
  • 13
-2

To me, changing it like this makes it less complex to understand. Although to some will be less readable. Also it is a little faster.

function testFunc() {
 var step = getModel('step');
 if(step==1){this.editTask(); this.updateStep(0); return true;} 
 if(step==2){this.initTask; return true;} 
 if(step==3){this.name==='add' ? this.add() : this.edit();return true;}
 return false;
}
Jan
  • 21
  • 2