0

So I'm currently learning JavaScript and the lecturer who taught the course I'm currently watching constantly talks about reducing cyclomatic complexity. He uses a few tricks for doing so which I cannot understand. I think I understand why a switch statement replaced by a map reduces the cyclomatic complexity - from many possible cases there is only one. However, he has been doing something like the following to reduce cyclomatic complexity:

if (condition) {
    arr.push(element);
} else {
    arr.pop();
}

becomes:

const actionsMap = {true: "push", false: "pop"};    
arr[actionsMap[condition]](element);

I prefer the second one, but how does it reduce the cyclomatic complexity? There are still two cases which are now only called in another way. If that really reduces cyclomatic complexity, can every if be replaced with a map with true and false keys to reduce cyclomatic complexity? He also uses || and && to reduce the cyclomatic complexity, but I've seen them listed as part of the cyclomatic complexity. How does this reducing of cyclomatic complexity work? Isn't it just a different shape for the same condition?

Edit: I do realize there are switch statements which cannot be so easily replaced by a map. Thank you for pointing out the reduced maintainability, but my question is: How does the reduction of complexity work here? Why is the actionsMap better than the if. Is it not a condition? Also, how do the logical operators reduce complexity? 

return (a>5 && a) || a-1;

How is this better than the following in terms of cyclomatic complexity:

if (a>5) return a;
else return a-1;

I know the examples are not of production quality code, but they illustrate my point pretty well.

asharpharp
  • 169
  • 1
  • 9
  • 4
    that second code is an unreadable too-clever mess. – dandavis Jun 01 '20 at 19:18
  • 1
    There is also something else to consider with all this. *Does the change to logic make it less readable?* Reducing complexity is all well and good, however if it comes at the cost of reduces readability then you are degrading the maintainability of your code. Anything can be taken too far to the extreme. You should consider all things in balance. – Taplar Jun 01 '20 at 19:19
  • also an object/map cannot replace all form of a js `switch`: each case is an _expression_ not a value, so you can `switch(true){...` and evaluate multiple expressions until the first condition is satisfied. – dandavis Jun 01 '20 at 19:21
  • 1
    ^ a map also cannot allow the logic to fall through to other cases, by not providing a `break;` – Taplar Jun 01 '20 at 19:22
  • ^ also you don't have to `break;` you can also `continue;` given there's something to continue around the `switch`, or you can `return`. And most important mix it up as you need it. – Thomas Jun 01 '20 at 19:24
  • @Taplar: are there any cases (no pun) where that fall-through behavior is actually desired? I always considered it a bug trap... – dandavis Jun 01 '20 at 19:28
  • @dandavis I can't say I remember a real world example. But I also wouldn't discount that it could be useful in some odd situation. – Taplar Jun 01 '20 at 19:29
  • "*He also uses || and && to reduce the cyclomatic complexity*" - I think that this lecturer did not understand the value of the cyclomatic complexity code metric. – Bergi Jun 01 '20 at 19:37
  • One of the things you have to understand about cyclomatic complexity is how it is measuered by most static analysis tools; the number of `if` and `switch` statements, combined with how deeply they are nested. So if you remove the `if`s and `switch`es, you lower the (reported) cyclomatic complexity. But, as others have warned, you could be increasing the cognitive load needed for humans to understand and maintain your code. – Heretic Monkey Jun 01 '20 at 19:44
  • @Heretic Monkey Thank you for explaining this. I can now ask a much better (I think) question. Does the cyclomatic complexity majorly affect branch and decision testing? Is it just an arbitrary measure made meaningless by such tricks or do these tricks mean the code will not be tested as a branching code? If the testing is affected, is it a good idea (only in terms of testing, not in terms of maintainability, as my goal is to understand cyclomatic complexity? – asharpharp Jun 01 '20 at 19:50
  • I don't think it *should* affect testing, as the code should be exercised whether as a result of an `if` clause or a bit shift. There *may* be some code coverage tools that are confused by such tricks (especially the second one), but that's why you employ humans, right? I wouldn't say it's arbitrary; if you have a single method with a cyclomatic complexity over, say, 50, that's probably too high and your method is likely doing too much. However, as a code reviewer, if I see a ton of code with tricks like those shown, I'd be making sure the test suite is *very* comprehensive. – Heretic Monkey Jun 01 '20 at 20:00
  • So is there any point to them? If they shouldn't affect testing and make the code less maintainable, are they of any use other than to make the life of reviewers, testers and everyone who looks at my code worse? – asharpharp Jun 01 '20 at 20:07

1 Answers1

0

IMO, your priority as a developer should be to write code that:

  • works
  • is easy for others to understand.

If your project is full of code fragments like the second example, the next person who comes along to maintain it is going to have a much harder time. It's a nice demonstration of how to use an alternative pattern to achieve the same result, but to me it's overkill for replacing a simple if/else.

Ethan Lipkind
  • 1,136
  • 5
  • 7