How to reduce if/else complexity?

Question

I have some if/else statements nested but I want to reduce their overhead.

In the example, I'm evaluating from which dropdown has an li item been clicked, and if this li item is the first (currentIndex === 0).

The code:

if (parentIndex === 1) {
  // But its own index is 0
  if (currentIndex === 0) {
    parentIndex = 2;
    updatedIndexPos = array[1];
    mainIndex =list_2.indexOf(updatedIndexPos);
    // If the previous line is -1:
    if (mainIndex === -1) {
      parentIndex = 1;
      updatedIndexPos = filterIndexPos;
      mainIndex = list_1.indexOf(updatedIndexPos);
    }
  } else {
    mainIndex = list_1.indexOf(mainIndexPos);
  }
} else if (parentIndex === 2) {
  // But its own index is 0
  if (currentIndex === 0) {
    parentIndex = 1;
    updatedIndexPos = array[0];
    mainIndex = list_1.indexOf(updatedIndexPos);
    // If the previous line is -1:
    if (mainIndex === -1) {
      parentIndex = 2;
      updatedIndexPos = filterIndexPos;
      mainIndex = list_2.indexOf(updatedIndexPos);
    }
  } else {
    mainIndex = list_2.indexOf(mainIndexPos);
  }
}

Looking at it there's a lot of reused code and eslint is giving me a complexity of 7 for it. I've tried breaking it into smaller functions and passing args but still haven't managed to solve the complexity of this code block.

As your code works but you're looking for specific improvements, I recommend posting this to [codereview.se], which exists for the purpose of reviewing and improving working code. Please read their [How to Ask a Good Question](https://codereview.stackexchange.com/help/how-to-ask) as it differs somewhat from Stack Overflow. — msanford, May 03 '18 at 12:42
Try read about state machines example https://blog.markshead.com/869/state-machines-computer-science/ if you are also using React like frameworks it gives you lot of ideas and nice add to your toolset. — Jimi Pajala, May 03 '18 at 12:46
This portion of my function has eslint returning a complexity of 7. Code review passes a complexity of 6 or less, so I want to reduce this piece down as much as I can. It's not complex in a real sense, but by the linting standards it is — Rebecca O'Riordan, May 03 '18 at 13:04
Ok, let me re-phrase that comment. [The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming.](https://en.wikiquote.org/wiki/Donald_Knuth) That having a random score of 7 when it "should be" 6 is premature optimisation. Cyclic complexity is a flawed measure of code quality. — Liam, May 03 '18 at 13:18

score 3 · Answer 1 · answered May 03 '18 at 12:56

It's often helpful to model logic as state-machine. So you have defined states and transitions between them.

var action = 'foo';
var actionParams = {...};
var currentState = getState({parentIndex: parentIndex, ...});
if (currentState.canPerform(action)) {
  currentState.perform(action, actionParams);
}

Evan Trimboli · Accepted Answer · 2018-05-03T12:48:09.593

2

Try and extract out the common portions, something like (untested):

if (parentIndex === 1) {
  potentialNewParent = 2;
  potentialUpdatedIndexPos = array[1];
  nullParentIndex = 1;
  mainList = list_1;
  otherList = list_2;
} else {
  // Do the same
}

if (currentIndex === 0) {
  parentIndex = potentialNewParent;
  updatedIndexPos = potentialUpdatedIndexPos;
  mainIndex = mainList.indexOf(updatedIndexPos);
  // If the previous line is -1:
  if (mainIndex === -1) {
    parentIndex = nullParentIndex;
    updatedIndexPos = filterIndexPos;
    mainIndex = otherList.indexOf(updatedIndexPos);
  }
} else {
  mainIndex = otherList.indexOf(mainIndexPos);
}

edited May 03 '18 at 12:48

answered May 03 '18 at 12:43

Evan Trimboli

29,900
6
45
66

1

Thanks Evan - I will try your solution. I have been trying to get these commonalities together but this looks more like it – Rebecca O'Riordan May 03 '18 at 12:47
2

The above is untested, just to demonstrate the general idea. – Evan Trimboli May 03 '18 at 12:48
I implemented a working version of this but eslint is still catching me with the nested `if (mainIndex === -1) {...}` statement! – Rebecca O'Riordan May 03 '18 at 13:29
1

The default max complexity before it should warn you is 20, I think you just have too strict settings for linting. – FINDarkside May 03 '18 at 13:42
@FINDarkside yeah the complexity settings is set at 6 for this project...it's very strict. Thanks! – Rebecca O'Riordan May 03 '18 at 13:42
1

You could split it up into functions, at this point it's somewhat arbitrary. – Evan Trimboli May 03 '18 at 13:56
I chose this as the accepted answer as it brought me closest to my goal. Thanks Evan! – Rebecca O'Riordan May 03 '18 at 16:11

score 1 · Answer 3 · answered May 03 '18 at 12:55

1

Assuming parentIndex is always 1 or 2, it can be simplified a lot. As I have no idea what it's supposed to do, I haven't tested this and the variable names might not be fitting:

const mainList = parentIndex === 1 ? list_1 : list_2;
const secondaryList = parentIndex === 1 ? list_2 : list_1;

if (currentIndex === 0) {
  parentIndex = parentIndex === 1 ? 2 : 1;
  updateIndexPos = array[parentIndex - 1];
  mainIndex = secondaryList.indexOf(updatedIndexPos);
  if (mainIndex === -1) {
    parentIndex = parentIndex === 1 ? 2 : 1;
    updatedIndexPos = filterIndexPos;
    mainIndex = mainList.indexOf(updatedIndexPos);
  }
} else {
  mainIndex = mainList.indexOf(mainIndexPos);
}

answered May 03 '18 at 12:55

FINDarkside

2,102
1
18
26

Thanks Fin, the parentIndex is always going to be 1 or 2 for this feature. I'm currently trying to implement @Evan Trimboli's answer below but will look at this too – Rebecca O'Riordan May 03 '18 at 13:02
Looks like it's mostly the the same solution, I just used ternary operations to save 16 lines of code compared to his code. Not that saving lines is always good, but I'd say that in this case it is. – FINDarkside May 03 '18 at 13:07

Kerlos Bekhit · Answer 4 · 2018-05-03T13:41:52.583

0

Assuming parentIndex could only be 1 or 2, could this work for you?

var elements = [2, 1];
if ((parentIndex === 1 || parentIndex === 2) && currentIndex === 0) {

  oldParentIndex = parentIndex;
  parentIndex = elements[oldParentIndex-1];
  updatedIndexPos = array[oldParentIndex%2];
  mainIndex = this["list_"+parentIndex].indexOf(updatedIndexPos);

  if (mainIndex === -1) {
    parentIndex = oldParentIndex;
    updatedIndexPos = filterIndexPos;
    mainIndex = this["list_"+oldParentIndex].indexOf(updatedIndexPos);
  }
} else if ((parentIndex === 1 || parentIndex === 2) && currentIndex !== 0) {
  mainIndex = this["list_"+parentIndex].indexOf(updatedIndexPos);
}

edited May 03 '18 at 13:41

answered May 03 '18 at 13:16

Kerlos Bekhit

128
1
7

Using eval for this is completely unecesary and a bad idea. https://stackoverflow.com/questions/86513/why-is-using-the-javascript-eval-function-a-bad-idea. this["list_"+parentIndex] should do the same. – FINDarkside May 03 '18 at 13:19
@FINDarkside it's still a viable solution, not the best, but viable. And could you help me understand how could eval() create security issues? – Kerlos Bekhit May 03 '18 at 13:26
eval will run the code you give to it. If you do eval(username), I can run any code I want on everyone's browsers by changing my username. While this particular code doesn't contain user generated content, it's still extremely wrong (and slow) tool for the job. this["list_"+parentIndex] is the correct way to do it. – FINDarkside May 03 '18 at 13:35

How to reduce if/else complexity?

4 Answers4